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

./wpt tests-affected can list tests when ./wpt test-jobs doesn't list stability #13937

Open
foolip opened this issue Nov 6, 2018 · 8 comments

Comments

@foolip
Copy link
Member

foolip commented Nov 6, 2018

An oddity discovered in #13936 which by the looks of it isn't accidental, but still probably worth addressing.

Among the PRs I looked at, these didn't trigger stability but did have affected tests.

#13611 and #13877 (its revert) both modified idlharness.js, but all of resources/ is excluded:

"!resources/*",
. ./wpt tests-affected still listed 150 affected tests, however. @lukebjerring, thoughts on this?

#13733 modified payment-request/META.yml, which is excluded, but META.yml is mentioned in a lot of tests in payment-request/. This isn't really a problem, though.

An open question is whether using ./wpt test-jobs --includes stability is necessary at all, or if it's better to just call ./wpt tests-affected. An upside of ./wpt test-jobs is that it doesn't require the manifest and so is faster.

@gsnedders
Copy link
Member

I can't find where this comes from originally (it predates the wpt tool IIRC), but it was because testharness.js caused every single testharness.js to be rerun. You can imagine that didn't work well…

@foolip
Copy link
Member Author

foolip commented Nov 6, 2018

Right, so the question is if jobs.py should just exclude resources/testharness*, instead of all of resources/.

@gsnedders
Copy link
Member

Arguably, once we move stability on to something with more capacity, we should run everything using testharness.js for every change of it.

@jgraham
Copy link
Contributor

jgraham commented Nov 12, 2018

That seems a bit academic at the moment; not only would we need more capacity we'd need some good way to figure out how many chunks to run to get the results in a reasonable amount of time (and certainly less than the timeout of the runner). Irrespective of capacity, we haven't got any good solution for that at the moment, and getting one is some way off.

@gsnedders
Copy link
Member

@jgraham Definitely. That said, at least doing one run so regressions could be surfaced would be useful.

@gsnedders
Copy link
Member

(Though I guess that's basically just #13263.)

@qiuzhong
Copy link
Contributor

@foolip , I got this error when running ./wpt affected-tests:

$ ./wpt affected-tests
usage: wpt [-h] [--venv VENV] [--debug]
           {tc-download,run,test-jobs,branch-point,create,lint,serve,check-stability,update-expectations,manifest,install,make-hosts-file,manifest-download,files-changed,tests-affected}
           ...
wpt: error: argument command: invalid choice: 'affected-tests' (choose from u'tc-download', u'run', u'test-jobs', u'branch-point', u'create', u'lint', u'serve', u'check-stability', u'update-expectations', u'manifest', u'install', u'make-hosts-file', u'manifest-download', u'files-changed', u'tests-affected')

I thought you mean ./wpt tests-affected but typed the wrong order for this argument.

@foolip foolip changed the title ./wpt affected-tests can list tests when ./wpt test-jobs doesn't list stability ./wpt tests-affected can list tests when ./wpt test-jobs doesn't list stability Nov 26, 2018
@foolip
Copy link
Member Author

foolip commented Nov 26, 2018

@qiuzhong, thank, I've updated the title and description.

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

No branches or pull requests

4 participants