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

service-workers/service-worker/ directory is unnecessary? #18768

Closed
foolip opened this issue Aug 30, 2019 · 7 comments
Closed

service-workers/service-worker/ directory is unnecessary? #18768

foolip opened this issue Aug 30, 2019 · 7 comments

Comments

@foolip
Copy link
Member

foolip commented Aug 30, 2019

After #18745 and #18766, the top-level service-workers/ is pretty empty:
https://github.com/web-platform-tests/wpt/tree/d208ff4d3606a3f70ff84e282d56ecc9e52c794e/service-workers

All the tests are in the cache-storage/ or service-worker/ directories.

The service-worker directory at this point looks unnecessary. Plenty of other directories refer to it, and it leads to rather long URLs like /service-workers/service-worker/resources/test-helpers.sub.js.

@asutherland @mkruisselbrink @mattto @wanderview, opinions?

If someone wants to take this on, in addition to doing the renaming carefully I'd recommend:

  • doing a full run (Ability to run all tests for some PRs #13263) with the changes to confirm no regressions
  • somehow making sure that in-flight changes here in WPT or downstream end up using the new path, maybe by adding a temporary lint forbidding the string "/service-workers/service-worker/"
@wanderview
Copy link
Member

This seems like a lot of churn in both the github repo and browser-specific annotation files for little gain. Is the cost really worth the pay off of changing it?

Also, I'd note its handy (for me at least) to be able to run service-workers/cache-storage separately from service-workers/service-worker. So I kind of like the separate subpaths.

@asutherland
Copy link
Contributor

I share wanderview's trade-off concerns. While it's possible all the automation in play for Gecko might properly show moves as moves (we use mercurial for revision control which does not infer moves), it might not, and that breaks blame which is exceedingly useful to us. But things like intermittent failure bugs filed against specific paths will definitely not automatically update.

@mkruisselbrink
Copy link
Contributor

Agree with the churn comment, although on the other hand having to live with in hindsight suboptimal decisions from the past indefinitely seems unfortunate as well.

Also, I'd note its handy (for me at least) to be able to run service-workers/cache-storage separately from service-workers/service-worker. So I kind of like the separate subpaths.

I assumed the proposal would end up with separate top-level service-workers and cache-storage top-level directories? Which at least would make a lot of sense if (when?) we move cache storage to its own spec.

@foolip
Copy link
Member Author

foolip commented Aug 30, 2019

Both Gecko and Chromium do handle renames in WPT import, and I think that's important so that infra limitations aren't the main reason to avoid renaming. But there are of course some rough edges, like https://crbug.com/801357.

@foolip
Copy link
Member Author

foolip commented Aug 30, 2019

(Submitted too early.)

But more importantly, if you who maintain the test suite think it's fine as is, then there's no problem, and I'll close this.

@foolip foolip closed this as completed Aug 30, 2019
@foolip
Copy link
Member Author

foolip commented Aug 30, 2019

I assumed the proposal would end up with separate top-level service-workers and cache-storage top-level directories? Which at least would make a lot of sense if (when?) we move cache storage to its own spec.

I never wrote the PR, but my intention was to just move service-workers/service-worker/* to service-workers/ and leave service-workers/cache-storage/ alone. So running just service-workers/cache-storage/ would be easy.

(It's also possible to run service-workers/ except service-workers/cache-storage/ using the --exclude flag for ./wpt run, but it's not something most people will discover.)

@asutherland
Copy link
Contributor

I'd definitely be open to a cleanup once the cache API is extracted out to its own spec.

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