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.

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

Split tests in Workflow.cpp into multiple files #2773

Merged
merged 10 commits into from
Dec 16, 2022

Conversation

florelis
Copy link
Member

@florelis florelis commented Dec 15, 2022

I've always disliked how big this file is, having tests for all flows. I didn't want to add even more to it for pinning, so I'm splitting it up :)

  • Created multiple files for each workflow and moved the appropriate tests there based on the tags they had. For example, moved tests tagged [ExportFlow] to ExportFlow.cpp. Tests that were the only ones with that tag, I left in Workflow.cpp
  • Created WorkflowCommon.h and WorkflowCommon.cpp with just the shared test source/context definition, and common task overrides.
  • Cleaned up the #includes and usings
Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner December 15, 2022 21:05
@florelis
Copy link
Member Author

florelis commented Dec 16, 2022

It would be so nice if git could see that I'm just moving code around and it could apply the changes automatically instead of having to do it by hand :(

If we want to take this, can we merge it before I get more conflicts?

The test failures from last run seem to have been a consequence of not having #2767, which only hit the tests once they were executed in a different order. I was wrong :p

return result;
}

SearchResult WorkflowTestCompositeSource::Search(const SearchRequest& request) const
Copy link
Member Author

Choose a reason for hiding this comment

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

I also want to split this function, I may do it later in another PR.

@florelis
Copy link
Member Author

The actual reason for the test failures was fun to find. There were some test hook overrides that were being set and never cleared when the test using them was done. So subsequent tests would still get the behavior from the override. Moving the tests around changed the order and revealed that. Since it was about a test hook override set at runtime, it would only show if executing the two relevant tests in the same invocation and in a specific order. Not having the fix for export settings was just a red herring. (I'll be mad if after this the tests still fail...)

@florelis florelis merged commit c3b8d54 into microsoft:master Dec 16, 2022
@florelis florelis deleted the splitWorkflowTests branch December 16, 2022 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants