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

Refactor test workflow source search #2782

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

florelis
Copy link
Member

@florelis florelis commented Dec 19, 2022

Another refactor that nobody asked for, for something that has been bothering me for a long time and I couldn't ignore while working on something else... (I swear I am also working on the stuff I'm supposed to!)

This change breaks up the huge WorkflowTestCompositeSource::Search() function, which was a long series of "if query is 'package' then add package to search results" (and which had a fun compilation warning about using too much space in the stack). I'm doing this by creating objects containing the matching query string and the modifications made to the search results in case of a match. Then, the search function can just iterate over these objects and modify the results when appropriate.

On each test, we now declare which results we want to get and we can use the query strings from these objects instead of duplicating the strings on each place they are needed.

I couldn't come up with a good name for these objects, so I'm expecting feedback on that if this change is something we want.

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner December 19, 2022 18:05
Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@florelis florelis merged commit 0a42851 into microsoft:master Jan 9, 2023
@florelis florelis deleted the splitTestSearch branch January 9, 2023 14:05
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