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

Special case testdriver.js when detecting affected tests #15485

Closed
Hexcles opened this issue Feb 20, 2019 · 7 comments · Fixed by #24745
Closed

Special case testdriver.js when detecting affected tests #15485

Hexcles opened this issue Feb 20, 2019 · 7 comments · Fixed by #24745

Comments

@Hexcles
Copy link
Member

Hexcles commented Feb 20, 2019

IIRC, testharness.js is treated specially in the heuristics for detecting affected tests (whenever it's modified almost all tests are affected). I think we need to do the same for testdriver.js as its use increases.

@foolip
Copy link
Member

foolip commented May 10, 2019

@LukeZielinski can you take a look at this? Does touching testdriver.js currently cause stability checks to time out, or how much margin do we have?

A similar case is idlharness.js where @lukebjerring added code to run more affected tests, but in that case we don't expect the number of tests to ever go much beyond the total number of specs/directories.

@lukebjerring
Copy link
Contributor

#13392 is the change mentioned

@LukeZielinski
Copy link
Contributor

So empirically (#16852) the stability checks were not run for a no-op change to testdriver.js. Although it's kind of surprising, since (as you said) only resources/testharness* is excluded in testfiles.py.

Is there some other configuration at play here?

@foolip
Copy link
Member

foolip commented Jun 5, 2019

@jgraham do you know why touching testdriver.js won't end up running any tests? I'm pretty sure it once did, and I haven't changed it or been aware of a change.

@foolip
Copy link
Member

foolip commented Sep 25, 2019

Ping @jgraham on #15485 (comment).

@gsnedders
Copy link
Member

@foolip they were in #20674; no idea why they weren't in #16852.

@stephenmcgruer
Copy link
Contributor

Marking roadmap due to inactivity (and lack of concern about inactivity)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants