for PTS v2 #1081

Konboi · 2025-07-23T08:38:53Z

To Enable PTS v2 Support

~~Add logic to check whether PTS v2 is enabled~~
~~When PTS v2 is enabled and no input is provided, enable Zero Input Subset (ZIS) by default~~
~~If ZIS has not been computed, automatically list files that look like test files~~
Add --get-tests-from-guess option

… the zero input subset automatically

Copilot

Pull Request Overview

This PR enables PTS (Predictive Test Selection) v2 functionality by detecting when a workspace is in "HANDS_ON_LAB_V2" state and implementing fallback behavior for automatic test collection when no tests are provided.

Key changes include:

Added workspace state caching and PTS v2 detection in the Launchable client
Implemented automatic test file collection using git ls-files when subset input is empty in PTS v2 mode
Refactored subset logic to handle multiple API requests with fallback behavior

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`launchable/utils/launchable_client.py`	Adds workspace state caching and PTS v2 detection method
`launchable/commands/subset.py`	Implements automatic test collection and refactors subset request handling with SubsetResult class
`tests/commands/test_subset.py`	Adds test coverage for PTS v2 behavior with multiple API requests

launchable/commands/subset.py

tests/commands/test_subset.py

…aces

ono-max · 2025-07-25T03:51:21Z

@Konboi

Is this an urgent PR? Can I review this PR in the next week?

Konboi · 2025-07-25T04:34:43Z

@ono-max You can review this PR in the next week. Thanks

kohsuke · 2025-07-28T22:51:52Z

launchable/commands/subset.py

+            If the zero input subset response is empty and the workspace is enabled to PTS v2,
+            CLI will try to collect the test files automatically, and request the subset again.
+            """
+            if client.is_enabled_pts_v2() and self.is_get_tests_from_previous_sessions and len(subset_result.subset) == 0:


len(subset_result.subset) == 0 test is unnerving because an empty list can be a valid response, and nothing otherwise indicates that the empty list carries this special meaning that triggers this special "let's look for what looks like tests" behavior.

I'm not sure how best to fix this. I'm now also thinking test listed like this are unlikely to be good enough for the CI process, it's only good for local examination of PTS, so I'm also wondering how best to balance that as well.

I'm now tentatively thinking defining a dedicated option to collect tests from git ls-files is better. It avoids surprises, it makes the code cleaner, and it avoids the abstraction leak of "HANDS_ON_LAB_V2" workspace state.

More concretely, my proposal is to define --guess-tests (or maybe --get-tests-from-guess to signal the fact that it's mutually exclusive with other --get-tests-...) and do local test guess scan if and only if this flag is given.

As far as I remember, the story we discussed was as follows:

If PTS v2 is enabled for the workspace and no input is provided, the Zero Input Subset (ZIP) is enabled.

If there is no data available for ZIS, the server returns an error

As a fallback, files that look like tests are automatically detected.

If the case where --get-tests-from-guess is enabled, what would the flow like?

Automatically detect files that look like tests

Request a subset used by 1.

Would this mean the ZIS flow is skipped entirely?

We discussed it directly and decided to introduce a new option, --get-tests-from-guess without any fallback behavior

kohsuke · 2025-07-28T22:53:11Z

launchable/utils/launchable_client.py

+        state = self._get_workspace_state()
+        return state.get('fail_fast_mode', False)
+
+    def is_enabled_pts_v2(self) -> bool:


is_pts_v2_enabled would be grammatically correct

ono-max

Although there are some comments from KK, you changed based on my comments. So, I'll approve this PR not to block you.

kohsuke · 2025-07-28T23:18:21Z

launchable/commands/subset.py

+            original_subset = subset_result.subset
+            original_rest = subset_result.rest


This code is rather unnerving in so many ways:

The name 'original_xyz' suggests this variable will be mutated later, and yet somehow we still need to maintain the distinction (which suggests leaky abstraction -- why are we needing to mutate this, and why do we need to tell them apart)?

Yet the list is not copied, which suggests the mutation code will not just manipulate the list, but rather it clones or creates a whole new list.

And yet no such mutation seems to be happening later.

I'm currently guessing this is just a refactoring remnant. If so, let's remove this. If not, please share with me the motivation behind this, so that I can work on the follow-up PR to do it differently.

launchable/commands/subset.py

kohsuke · 2025-07-28T23:40:41Z

launchable/utils/launchable_client.py

+
+    def is_enabled_pts_v2(self) -> bool:
+        state = self._get_workspace_state()
+        return state.get('state', "") == "HANDS_ON_LAB_V2"


I mentioned "the abstraction leak of HANDS_ON_LAB_V2", let me expand it here a bit.

"PTS v2" is amorphous project. So when you define a flag like this, I worry about how the client of this code can easily drift off from what the server side means with this flag. More so if you think about the fact that the CLI is a packaged software and therefore it's more "stiff" than the server side code, meaning we have less control over how long it lives.

And this starts to feel even more out of place if you think about the fact that the test selection logic is really supposed to be a black box from the CLI perspective anyway. Where and why would the CLI need to know how exactly our service is selecting tests? It asks for scrutiny.

AFAICT, the only reason this is needed is in order to trigger implicit local test collection behavior. For mostly unrelated reasons I'm now thinking that's problematic anyway. So I'm inclined to suggest we shouldn't have the is_enabled_pts_v2 method.

If we do decide to keep, I'd advise against making this another constant to the broken 'workspace state' enum. Enum means those state constants are mutually exclusive, but this one is not -- an ACTIVE_PAYING workspace might have a V2 PTS, and similarly PUBLIC_OSS_PROJECT might have a V2 PTS.

(For that matter, I think we better clarify the meaning of "workspace state". ARCHIVED is probably not an enum state either)

Thanks.
I think using an enum is fine, but I believe it would be better to have the value returned from the server, like isFailFastMode. So I’ll fix it in this PR

kohsuke

I left so many comments, but but in hindsight, maybe it's me wondering down a tangential path.

In the interest of not blocking the critical effort, I think the only clear "bug" is the one comment around subprocess.CalledProcessError, and I can work on a separate PR to try to improve the relevant code a bit.

…discovery

Konboi · 2025-07-29T04:34:19Z

@kohsuke I fixed some points based on your feedback. Could you review again, just in case?
Especially this part #1081 (comment)

This logic should be promoted further up ideally.

This is a configuration error so I think it makes sense to make this fatal.

- Emitting a warning is a common pattern. This needs to be promoted up. - `git ls-files` can result in other types of exceptions thrown - Not finding anything in `git ls-files` should trigger a warning

Konboi · 2025-07-30T00:23:08Z

launchable/commands/subset.py

        tracking_client.send_error_event(event_name=event, stack_trace=msg)
        sys.exit(1)

+    if is_get_tests_from_guess and is_get_tests_from_previous_sessions:


Konboi · 2025-07-30T00:28:13Z

launchable/commands/subset.py

                if self.input_given:
                    print_error_and_die("ERROR: Given arguments did not match any tests. They appear to be incorrect/non-existent.", Tracking.ErrorEvent.USER_ERROR)  # noqa E501
-                if client.is_enabled_pts_v2():
+                if client.is_pts_v2_enabled():


Because we decided to add --get-tests-from-guess option instead auto enabling the zero input subset with fallback behavior #1081 (comment)

Also, I removed the code 5f1c058#diff-4c62e35fb4e3b2b6bff8593107cdddd1fd47eff90293edc9d838c9b2f3cff94aL603-L605 but it has reverted

I'll remove this if condition.

sonarqubecloud · 2025-07-30T02:18:43Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Konboi force-pushed the subset-file-auto-collection branch from f914855 to 9a8d922 Compare July 24, 2025 00:41

Konboi added 3 commits July 24, 2025 09:54

to be able to check that pts v2 is enabled or not

07f2fa6

if the workspace is enabled pts v2 and input is zero, the cli enables…

d66f829

… the zero input subset automatically

add test case

fafe161

Konboi force-pushed the subset-file-auto-collection branch from 9a8d922 to d9641c3 Compare July 24, 2025 08:44

Konboi requested a review from Copilot July 24, 2025 08:52

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

Konboi force-pushed the subset-file-auto-collection branch 3 times, most recently from 190590d to 26e857b Compare July 25, 2025 01:17

Konboi requested a review from Copilot July 25, 2025 01:21

Copilot AI reviewed Jul 25, 2025

View reviewed changes

Konboi added 9 commits July 25, 2025 11:37

use instance field

c0bfd63

introduce SubsetResult class to handle subset result

c41d9a6

introduce auto test file like collecting logic for hands_on_v2 worksp…

61cce47

…aces

fix type

4a347de

to support 3.6

86093c9

add error handling

f566f6f

fix comments

728c707

to check more strictly

4f7323d

add test case for pts v2 usecase

707f0c1

Konboi force-pushed the subset-file-auto-collection branch from 26e857b to 707f0c1 Compare July 25, 2025 02:38

Konboi changed the title ~~To enable PTS v2~~ for PTS v2 Jul 25, 2025

fix comments

c36d308

Konboi marked this pull request as ready for review July 25, 2025 03:06

Konboi requested review from kohsuke and ono-max July 25, 2025 03:06

kohsuke reviewed Jul 28, 2025

View reviewed changes

ono-max approved these changes Jul 28, 2025

View reviewed changes

kohsuke reviewed Jul 28, 2025

View reviewed changes

launchable/commands/subset.py Show resolved Hide resolved

kohsuke reviewed Jul 28, 2025

View reviewed changes

launchable/commands/subset.py Show resolved Hide resolved

kohsuke reviewed Jul 28, 2025

View reviewed changes

kohsuke approved these changes Jul 29, 2025

View reviewed changes

Konboi added 6 commits July 29, 2025 09:22

rename method name

fd568e8

rm unnecessary code

eec70c0

don't use state directly

09360e2

need check to catch exception

e3abd46

introduce --get-tests-from-guess option instead of fallback and auto …

5f1c058

…discovery

fix api call count

e3fdfdf

Konboi requested a review from kohsuke July 29, 2025 04:33

kohsuke added 3 commits July 29, 2025 10:17

[refactor] docstring must be at the top of a function

05ce7e4

[refactor] report and error and die is a common pattern

4b0d59e

This logic should be promoted further up ideally.

These two options are mutually exclusive

244b82f

This is a configuration error so I think it makes sense to make this fatal.

kohsuke approved these changes Jul 29, 2025

View reviewed changes

kohsuke added 3 commits July 29, 2025 13:31

Improved error handling

a9dd320

- Emitting a warning is a common pattern. This needs to be promoted up. - `git ls-files` can result in other types of exceptions thrown - Not finding anything in `git ls-files` should trigger a warning

They should probably be an error in the fail-fast mode

5228a1e

fixup

9ca63ee

kohsuke force-pushed the subset-file-auto-collection branch from fdaafc4 to 9ca63ee Compare July 29, 2025 20:31

Konboi commented Jul 30, 2025

View reviewed changes

removed, since it was unintentionally reverted

445601d

Konboi merged commit 6995783 into main Jul 30, 2025
15 checks passed

Konboi deleted the subset-file-auto-collection branch July 30, 2025 03:43

github-actions bot mentioned this pull request Jul 30, 2025

Release for v1.107.5 #1090

Merged

		original_subset = subset_result.subset
		original_rest = subset_result.rest

for PTS v2 #1081

for PTS v2 #1081

Uh oh!

Conversation

Konboi commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ono-max commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Konboi commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Konboi Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ono-max left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohsuke left a comment

Choose a reason for hiding this comment

Uh oh!

Konboi commented Jul 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 30, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Konboi commented Jul 23, 2025 •

edited

Loading

ono-max commented Jul 25, 2025 •

edited

Loading

Konboi Jul 29, 2025 •

edited

Loading