Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add category for extra tests to run in PRs #1046

Merged
merged 2 commits into from
Apr 18, 2019
Merged

Add category for extra tests to run in PRs #1046

merged 2 commits into from
Apr 18, 2019

Conversation

kewillford
Copy link
Member

This adds another category to the tests to allow the extra tests that are currently only ran during to the full suite to be ran so that we can add a PR build for just them without having to run the full suite since the other tests are already ran during a PR build.

@kewillford
Copy link
Member Author

/azp run PR - Windows - Extra

@azure-pipelines
Copy link

For the Azure DevOps organization gvfs, no matching pipelines using the Azure Pipelines app were found for this pull request.

@kewillford kewillford force-pushed the pull-out-extra-tests branch from f00c4be to b942840 Compare April 17, 2019 18:38
@kewillford
Copy link
Member Author

/azp run PR - Windows - Extra

@azure-pipelines
Copy link

For the Azure DevOps organization gvfs, no matching pipelines using the Azure Pipelines app were found for this pull request.

@kewillford
Copy link
Member Author

/azp run PR - Windows - Extra

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kewillford
Copy link
Member Author

/azp run Windows - Full Functional Tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kewillford
Copy link
Member Author

/azp run PR - Windows - Extra

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kewillford
Copy link
Member Author

/azp run Windows - Full Functional Tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wilbaker
Copy link
Member

@kewillford latest changes look good to me

jamill
jamill approved these changes Apr 18, 2019
View reviewed changes
Copy link
Member

@jamill jamill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. My only question is around naming - are these tests in the "full test suite only" / "extra-coverage" because they are long running? If so, maybe name this level of tests "long-running". Or are these in this category because they are lower priority? or...

@kewillford
Copy link
Member Author

@jamill I don't remember any official discussion on the tests that were added to this group so I would have to go through them and see. For now I like them as simply extra coverage tests and we can sort out why and if they need a different name some other time.

jrbriggs
jrbriggs approved these changes Apr 18, 2019
View reviewed changes
Copy link
Member

@jrbriggs jrbriggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! It'll be nice to not accidentially break the full suite.

@kewillford kewillford merged commit 1ae71d4 into microsoft:master Apr 18, 2019
@kewillford kewillford deleted the pull-out-extra-tests branch April 18, 2019 14:37
@wilbaker
Copy link
Member

This makes sense. My only question is around naming - are these tests in the "full test suite only" / "extra-coverage" because they are long running? If so, maybe name this level of tests "long-running". Or are these in this category because they are lower priority? or...

@jamill @kewillford here is the description/motivation from the PR in Azure Repos (302803):

This PR does the following:

  • Add the ability to mark a test with Category([Categories.FullSuiteOnly]) to indicate that it should not run in normal PR tests, only in the full suite of tests
  • Marked a few test fixtures with that category. Tried to mark the tests that are slow, and also unlikely to catch errors in core scenarios, so that it's relatively safe to skip running them in all PRs. Folks who want full coverage should still run the optional full test suite in their PRs.
  • Cleaned up the test outputs so that start/end messages are not interleaved with other tests.
  • After the tests complete, print out stats on how long each fixture and each test case took to run, so that we can evaluate which ones are worth investigating for performance
    Along with these changes, I also plan to make the full suite of tests run after every merge into master.

Another possibility to explore (as future work) is to divide the tests into various categories of roughly equal runtime, and then create a build definition per category and make them all required. The advantage of that approach is that if the build machines are all free, you can complete an individual PR much faster by spreading the tests over multiple machines.

@kewillford kewillford added this to the M151 milestone Apr 18, 2019
@jrbriggs jrbriggs added affects: engineering Keeping the engineering system healthy affects: tests type: enhancement labels Apr 19, 2019
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 4, 2020
@chrisd8088
FunctionalTests: remove ExtraCoverage category
162e8bc 
A limited subset of functional tests are marked as belonging to
the ExtraCoverage category, which was introduced in the
microsoft/VFSForGit#1046 PR as an enhancement over the
FullSuiteOnly category from the Azure Repos PR 302803.
The intent was to make the default functional test run faster
by excluding some less-fragile tests.

However, in Scalar, many of these tests are also marked with
NeedsUpdatesForNonVirtualizedMode and therefore do not run at all,
while the ones which remain have been running in CI on the macOS
platform but not Windows, because Scripts/Mac/RunFunctionalTests.sh
passes the --full-suite option.

In order to simplify the CI and functional test framework,
we eliminate the ExtraCoverage category entirely, which
ensures all working tests run on all platforms in CI.
@chrisd8088 chrisd8088 mentioned this pull request Sep 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrbriggs jrbriggs jrbriggs approved these changes

@jamill jamill jamill approved these changes

@wilbaker wilbaker wilbaker approved these changes

Assignees
No one assigned
Labels
affects: engineering Keeping the engineering system healthy affects: tests type: enhancement
Projects
None yet
Milestone
M151
Development

Successfully merging this pull request may close these issues.
4 participants
@kewillford @wilbaker @jrbriggs @jamill