-
Notifications
You must be signed in to change notification settings - Fork 13
Add new command 'launchable compare subsets' #1107
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
Conversation
| changes.sort(key=lambda x: (abs(x[3]) if isinstance(x[3], int) else float('inf')), reverse=True) | ||
|
|
||
| # Display results in a tabular format | ||
| headers = ["Before", "After", "Order Change", "Test Path"] |
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.
Probably, "Order Change" sounds a little bit unclear to me. Can we rename it to the following names?
- After - Before
- Order Δ
- Order Diff
- Diff (After - Before)
- Δ (After - Before)
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.
I'll change it to 'After - Before'
| after_tests = f.read().splitlines() | ||
| after_index_map = {test: idx for idx, test in enumerate(after_tests)} | ||
|
|
||
| changes = [] |
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.
Can you please define the type for this variable?
e.g.
smart-tests-cli/launchable/utils/http_client.py
Lines 24 to 25 in e1ff1d3
| DEFAULT_TIMEOUT: Tuple[int, int] = (5, 60) | |
| DEFAULT_GET_TIMEOUT: Tuple[int, int] = (5, 15) |
|
With this command, we are trying to draw attention to tests that boosted in ranks. So it is critical that we sort the result like that. IOW, sort them in the ascending order of diff. |
|
"Test path" is a technical term in our product to refer to a certain structured test name, so I think it's best to avoid that, since what we are showing is not that. Maybe just "test"? |
I think we can name it 'Test' Btw these are the names used in
|
| if test in before_index_map: | ||
| before_idx = before_index_map[test] | ||
| order_diff = after_idx - before_idx | ||
| changes.append((test, before_idx + 1, after_idx + 1, order_diff)) |
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.
May I ask why you're using tuple, instead of map?
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.
I think tuple is a suitable choice for representing a fixed-size row in a table. It should be also slightly more efficient compared to a map in this case.
I declared the variable with types, and also added a comment to make it clear on what goes in the tuple.
|
I updated the PR so that the results are sorted in ascending order of the diff value. |
kohsuke
left a comment
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.
Woot!
This adds a new command
launchable compare subsetswhich can be used to compare two subset files.Example usage:
When there're new test paths in the subset-after.txt, new file paths will be a marked as 'NEW':
When there're deleted test paths between the two subsets, deleted test paths will be marked as 'DELETED':