[forge] Split out test suites from main #15033
Merged
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
⏱️ 1h 32m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
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.
This comment has been minimized.
This comment has been minimized.
perryjrandall
force-pushed
the
perry/split-forge-suites
branch
from
237fba6 to
339f2cd
Compare
This comment has been minimized.
This comment has been minimized.
perryjrandall
force-pushed
the
perry/split-forge-suites
branch
from
339f2cd to
2e80bb3
Compare
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.
There are a lot of tests and cruft in with the main forge cli Split them out into suites Test Plan: running forge on PR succeeds
perryjrandall
force-pushed
the
perry/split-forge-suites
branch
from
2e80bb3 to
5b2c544
Compare
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
|
aluon
approved these changes
Oct 21, 2024
| @@ -617,7 +518,7 @@ fn get_test_suite( | |||
|
|
|||
| // Otherwise, check the test name against the grouped test suites | |||
| if let Some(test_suite) = get_land_blocking_test(test_name, duration, test_cmd) { | |||
| return Ok(test_suite); | |||
| Ok(test_suite) | |||
There was a problem hiding this comment.
Should we also remove the return statements from the other branches to match this?
There was a problem hiding this comment.
Woops yeah i already fixed this in another commit forgot to pick
aluon
reviewed
Oct 21, 2024
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| pub mod dag; | ||
| pub mod db; |
There was a problem hiding this comment.
Probably for a future PR. Now that we have the test suites split up into different modules, it'd be great to have module-level docs for what each test suite is doing. @igor-aptos added some comments for forge-stable in #14775 , maybe we should move some of the docs to these files?
There was a problem hiding this comment.
Yeah in general i think these should live with the tests themselves not in docs on a forge workflow
banool
approved these changes
Oct 21, 2024
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.
There are a lot of tests and cruft in with the main forge cli
Split them out into suites
Test Plan:
running forge on PR succeeds