-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Williamhe/1504900 optimize by restructuring test invocation #1438
Williamhe/1504900 optimize by restructuring test invocation #1438
Conversation
…t 3.1 sdk version" This reverts commit 2396a29.
…mhe/1504900-optimize-by-restructuring-test-invocation
value: githubactions | ||
- | ||
key: VsoFocal | ||
value: vso-focal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note:
JobNames cannot have a dash -
, docker images cannot have uppercases, and having underscore brings problems with hostnames. Which is why we have the key/value parameters as shown above.
{ | ||
GDIPlusLibrary_IsPresentInTheImage("github-actions"); | ||
GDIPlusLibrary_IsPresentInTheImage("github-actions-buster"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note:
Xunit has a limitation with invoking a specific category:parameter
so we went with this approach of wrapping a functions with a specific category:parameter
. The feature to filter by category:parameter
expected to be released in v3
of xunit, Per requests back from 2020.
xunit/xunit#2177 (comment)
xunit/xunit#1758 (comment)
xunit/xunit#2133
One thing I notice, I'm not sure if you're already on it, the validation "build and test image" jobs didn't actually include the build build images steps, while the nightly build did it. |
@@ -27,7 +27,7 @@ public PrepareEnvironmentCommandTest(ITestOutputHelper output) : base(output) | |||
{ | |||
} | |||
|
|||
[Fact] | |||
[Fact, Trait("category", "githubactions")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these are listed as github-actions
, some as githubactions
. Based on the ci.yaml and nightly.yaml, as well as the tests that actually ran, I'm not sure that the github-actions
tests are being included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note, the test counts between the Before and After nightly runs (from in this ticket description) are pretty different. Do you know what is accounting for those differences in counts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! All GitHub Actions category should be githubactions
. I've pushed a new change out
var imageTestHelper = new ImageTestHelper(); | ||
InstallsHugoVersionDynamically_UsingEnvironmentVariable_AndBuildsApp(imageTestHelper.GetGitHubActionsBuildImage()); | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(ImageNameData))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to get rid of the above two lines since we won't be running this test without calling it from a wrapping function? We should also be able to make this function private.
There are a couple of spots in the PR where this also occurs, curious if we can make this change throughout the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've removed MemberData
on methods that had the wrapper test method
Thanks @qianz2 I have updated the YAML file for this |
2 hour reduction. Reduced time from ~3.5hrs to ~1hr17 mins
This is achieved by building and testing BuildImages in their own Job which runs in parallel.
Before Parallelization:
After Parallelization:
- [ ] Proper license headers are included in each file.