add a lambda transactional-test, clean up test driver a bit to improve coverage of existing tests #15463
+1,262
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 47m total CI duration on this PR
|
brmataptos
changed the title
rahxephon89
approved these changes
Dec 4, 2024
| name: "baseline", | ||
| runner: |p| run(p, get_config_by_name("baseline")), | ||
| experiments: &[], | ||
| language_version: LanguageVersion::V2_2, |
There was a problem hiding this comment.
Whould we have a const like "LATEST_UNSTABLE_VERSION" for language and set it to V2_2 or it is just specific for v2.2?
There was a problem hiding this comment.
I really want a version like LanguageVersion::LAMBDAS but that seems like overkill.
There was a problem hiding this comment.
Oops, that one should not be V2_2 but should be stable. Hold on.
…aseline test config to ensure we are running baseline for all tests.
brmataptos
force-pushed
the
brm-add-lambda-tests
branch
from
605dff3 to
8625359
Compare
vineethk
reviewed
Dec 4, 2024
| @@ -17,8 +17,8 @@ use std::{ | |||
| }; | |||
|
|
|||
| const UNSTABLE_MARKER: &str = "-unstable"; | |||
| pub const LATEST_STABLE_LANGUAGE_VERSION: &str = "2.1"; | |||
| pub const LATEST_STABLE_COMPILER_VERSION: &str = "2.0"; | |||
| pub const LATEST_STABLE_LANGUAGE_VERSION: LanguageVersion = LanguageVersion::V2_1; | |||
There was a problem hiding this comment.
This code does not compile (try running cargo build from the aptos-core root directory or take a look at the failing CI).
There was a problem hiding this comment.
oops, checked tests only, forgot this is used elsewhere. added new consts LATEST_STABLE_X_VALUE instead of remaing the old one to avoid conflicts with existing code.
| @@ -29,37 +29,47 @@ struct TestConfig { | |||
| experiments: &'static [(&'static str, bool)], | |||
| /// Run the tests with language version 1 (if true), | |||
| /// or with latest language version (if false). | |||
| is_lang_v1: bool, | |||
| language_version: LanguageVersion, | |||
There was a problem hiding this comment.
Doc comments should be updated.
| @@ -109,6 +183,8 @@ const SEPARATE_BASELINE: &[&str] = &[ | |||
| "no-v1-comparison/assert_one.move", | |||
| // Flaky redundant unused assignment error | |||
| "no-v1-comparison/enum/enum_scoping.move", | |||
| "/lambda/", | |||
There was a problem hiding this comment.
Could we add a comment for these new entries like in the cases above? Otherwise, it seems like "flaky redundant unused assignment error" applies to these two entries.
vineethk
reviewed
Dec 5, 2024
| pub fn latest_stable() -> Self { | ||
| CompilerVersion::from_str(LATEST_STABLE_COMPILER_VERSION).expect("valid version") | ||
| pub const fn latest_stable() -> Self { | ||
| LATEST_STABLE_COMPILER_VERSION_VALUE |
There was a problem hiding this comment.
Not sure why we need this change. Now, to upgrade the latest stable language version, we need to change both LATEST_STABLE_LANGUAGE_VERSION and LATEST_STABLE_LANGUAGE_VERSION_VALUE, and make sure to keep them in sync.
Instead, in the previous version, you would only need to change the LATEST_STABLE_LANGUAGE_VERSION, and if it is set to the wrong string, tests would fails.
There was a problem hiding this comment.
@brmataptos Just to make sure you saw this comment. The PR is ready to go once this is addressed.
There was a problem hiding this comment.
We need a const value to use with the config structs in tests.rs. While we could use a string, that's needlessly indirect. I've The old definition of latest_stable() cannot be declared as const, due to the non-const nature of trait functions like from_str() (apparently const traits are currently "unstable", but also there may be types for which from_str() wouldn't be constant). In this case, defining a custom to_str() for CompilerVersion and LanguageVersion allows us to derive LATEST_STABLE_COMPILER_VERSION from LATEST_STABLE_COMPILER_VERSION_VALUE and have a const value we can store in the config.
vineethk
approved these changes
Dec 12, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
❌ Forge suite
|
✅ Forge suite
|
❌ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add a
move-compiler-v2/transactional-tests/tests/llambda/directory and a config for running lambda tests there.Changed
transactional-tests/tests/tests.rstobaselinewhich is run for most things (not/operator_eval/or/access_control/)How Has This Been Tested?
Ran tests.
Key Areas to Review
Please ignore contents of lambda test output, I pulled this out of the lambda stack to get the config fixed faster.
Type of Change
Which Components or Systems Does This Change Impact?
Checklist