search

web-platform-tests / rfcs

web-platform-tests RFCs
76 stars 64 forks source link

Integration of the Infrastructure Part of the Web Media API Test Suite 2018 into WPT #23

Closed louaybassbouss closed 3 years ago

louaybassbouss commented 5 years ago

This RFC discusses the integration of the Web Media API Test Suite 2018 into WPT.

Rendered

louaybassbouss commented 5 years ago

Any feedback on this PR?

gsnedders commented 5 years ago

@louaybassbouss Quite a lot of us are OOO at the moment. 😔

louaybassbouss commented 5 years ago

Thanks @gsnedders for the info

foolip commented 4 years ago

Hi @louaybassbouss, sorry that this has gone so long without attention. Pinging @web-platform-tests/wpt-core-team.

I've looked over https://github.com/cta-wave/WMAS and I see two parts: infrastructure changes and new tests. I assume you're proposing to integrate both of these, or would either in isolation be valuable?

Infrastructure

There are a few branches there, but it looks like wmas2018 is the main one and it branched from this repo at commit 4bdeca6b451519a7f60f592468600e0a6cbfc42b. Using git diff --diff-filter=AM (to ignore deletions) it looks like the important changes are:

Is running in a browser that doesn't support tabs the whole scope of these changes? Can you explain what the purpose of the node server here is, could the same not be achieved with wptserve?

New tests

I see some new tests in https://github.com/cta-wave/WMAS/tree/wmas2018-tests/wave-extra.

Any tests that are asserting things already required by existing web specs would be fine to just submit as pull requests, and "Devices MUST support deleting a cookie" is probably such a test, although I'd expect there to be existing cookie deletion tests somewhere.

It looks like a bunch of the tests are manual, which is understandable for a test that says "Please restart the device!" :) Manual tests won't be run and thus submitting them has pretty low impact. If you can find a good place to put these tests I think that'd be fine.

Most of the tests seem to be of the type "Devices MUST support at least 100 total cookies" or similar. These are a bit like stress tests, although not pushing the limits very far. All but one of the tests are about cookies, so pinging the lone reviewer of cookies in wpt, @mikewest, for thoughts on adding such tests. My own sense is that as long as the tests are well written and don't push the limits very far, they're fine. Tests that would be very slow or use a lot of resources I'd be more wary about.

Are there more new tests on some other branch?

louaybassbouss commented 4 years ago

Thanks @foolip the infrastructure changes are more important I would propose to handle them separately from the new tests. The main branch wmas2018 is the relevant branch for us. Below are comments to your questions

tools/wave: A server to assist the run of the web platform tests in a single window

Exactly we are targetting embedded devices like TVs and STBs where you can only run tests in a single window. Therefore, we shifted part of the test runner logic to the server which is written in Node.js. The Test Runner offers also a companion page where you can monitor/control the test session on the DUT. You can also list, view and compare test sessions and generate reports (using wptreport tool) through the companion page.

changes to resources/testharnessreport.js also for this purpose?

Yes, we modified this file in order to report the results of the current test to the Server and ask for the next test file in the current session.

a WaveProxyHandler in tools/wptserve/wptserve/handlers.py, also part of the same?

Yes, basically it forwards requests to the Node.js Server to avoid cross-origin request.

Is running in a browser that doesn't support tabs the whole scope of these changes?

Yes, browsers that don't support tabs are in scope but as I mentioned above, in CTA WAVE we are mainly targetting TVs and STBs.

Can you explain what the purpose of the node server here is, could the same not be achieved with wptserve?

The Server implements the whole logic to managing test runs (sessions) and generating the test reports. The tests are served as before via wptserve. The changes we made in resources/testharnessreport.js are responsible for the communication with the Server.

We can handle new tests in a seperate issue and focus in this RFC only on the infrastructure.

mikewest commented 4 years ago

Most of the tests seem to be of the type "Devices MUST support at least 100 total cookies" or similar. These are a bit like stress tests, although not pushing the limits very far. All but one of the tests are about cookies, so pinging the lone reviewer of cookies in wpt, @mikewest, for thoughts on adding such tests.

I guess they're fine, with the caveat that landing these tests in WPT is not going to create an obligation on behalf of browser vendors to continue supporting whatever limits are being validated. Folks seem enthusiastic about imposing various and sundry limitations/expirations on the ability to use document.cookie, for example. Likewise, the non-HTTPS variants might stop working if secure context restrictions expand.

It's unlikely that vendors will feel bound to keep these tests passing, and therefore likely that they'll be modified by one of the vendors as behaviors evolve. If you need these to be a stable conformance test for hardware you verify, it's probably a better idea to keep them in a repository you control.

foolip commented 4 years ago

We can handle new tests in a seperate issue and focus in this RFC only on the infrastructure.

@louaybassbouss that sounds good, can you rename this PR to make the limited scope clear? (The branch and file names already seem fine.)

My overall question about the running infrastructure is if the same could be achieved by modifications to wptrunner / wptserve implemented in Python, and what the drawbacks of doing that would be.

Also, how is communication with the TV/STB established in the first place? If you're using WebDriver I'd be optimistic that you could rejig the existing code base to avoid creating that extra window. However, if there's some manual step involved in connecting the test runner to the device under test, and there's no control protocol like WebDriver involved, then the changes would probably be more involved.

Hexcles commented 4 years ago

Most of the inner working is documented in the issue https://github.com/web-platform-tests/wpt/issues/16214

My understanding is that WebDriver is not used, and it'd be difficult to retro-fit the proposed model into wptrunner. I think it's fine to have an additional server backend to control the test execution but I hope it'd be implemented in Python instead of Node.js.

zcorpan commented 4 years ago

Looking at @jgraham's comment in https://github.com/web-platform-tests/wpt/issues/16214#issuecomment-489141605 - who will own the new infrastructure and tests?

I'd also like to see more details in "Risks". Adding new infrastructure, new dependencies (Node.js?), new tests, and new users to the project doesn't seem like it would be without risk.

foolip commented 4 years ago

@louaybassbouss do you have a clear idea of what extra details we're asking for?

louaybassbouss commented 4 years ago

@foolip sorry for the late reply on this yes for me it is clear what are the open questions and what to do from our side let me summarize:

Are there other points I missed?

foolip commented 4 years ago

@louaybassbouss that all sounds right.

I think the Node.js dependency is going to be the primary concern here, since we don't have any node dependencies so far, at least no runtime dependencies. The core functionality of being able to run on a browser that doesn't support tabs is one I think ought to be possible to support in wptrunner+wptserve with modest modifications, so I'd suggest exploring that, as the ongoing maintenance burden of that ought to be much smaller than a new server on a new tech stack.

This is not to say that a Node.js dependency is absolutely out of the question, just that it requires very strong justification.

gsnedders commented 4 years ago

Should we close this, as the RFC in its current state (or remotely close to it) isn't something we will accept? Per the above, we need a lot more detail in the RFC to evaluate what exactly is proposed here.

jgraham commented 4 years ago

FWIW I'm happy to leave this open for now, assuming that there's a path to agreement here.

louaybassbouss commented 4 years ago

@jgraham @gsnedders @foolip I update the risk section concerning the issue with the Node.js dependency. We need a final decision if Node.js is still an option or not. If not, the only option is to port the code to Python and integrate it into wpt-* tools, but this option is associated with additional effort and requires internal approval in the project.

foolip commented 4 years ago

@louaybassbouss I don't think that a runtime Node.js dependency is an option at this point, at least not for functionality that can plausibly be supported in the existing wptrunner code base.

Others may speak up if they disagree, of course.

jgraham commented 4 years ago

Yes, I agree with that. Taking a second HTTP server based on a tech stack that is otherwise not used in the project is just too much in terms of the additional maintaince burden and increased difficulty of deployment. However given the implementation issues are fixed, I'm positive about landing the featureset proposed by this change; there's no implied lack of desire to support the use cases here.

louaybassbouss commented 4 years ago

@foolip @jgraham @gsnedders thank you for your feedback and support. Following your recommendation, we are going to covert the Node.js source code to Python and integrate it into the wpt-* tools. We would like to use this channel for discussion, can we keep this PR open?

foolip commented 4 years ago

Glad to hear this is progressing! Continuing here works, or https://github.com/web-platform-tests/wpt/issues/new would also make sense. The upside of the latter is we can apply the appropriate labels, but it's a small upside.

louaybassbouss commented 4 years ago

Thanks @foolip, yes we will then create concrete new issues with appropriate labels.

louaybassbouss commented 4 years ago

Dear all, I am happy to announce that we finished the work on porting the Node.js code into Python. I just created a PR in the WPT repository. For more details please refer to the description of the PR. Please let me know if you have questions or you need clarifications.

stephenmcgruer commented 4 years ago

@louaybassbouss given that the associated PR for this (https://github.com/web-platform-tests/wpt/pull/21323) has now merged, can you update this RFC to reflect the current situation? I think important points are:

stephenmcgruer commented 4 years ago

@louaybassbouss ping, are you able to schedule some time to update this RFC to reflect the current situation?

louaybassbouss commented 4 years ago

@stephenmcgruer I will do it ASAP I need to sync with WAVE group about it before I comment. I that ok for you?

stephenmcgruer commented 4 years ago

@stephenmcgruer I will do it ASAP I need to sync with WAVE group about it before I comment. I that ok for you?

Sounds great, thanks :)

louaybassbouss commented 3 years ago

@stephenmcgruer sorry for the delay I just updated the RFC. Please check and let us know if you still have questions. @JohnRiv is the update ok for you?

JohnRiv commented 3 years ago

Thanks @louaybassbouss, I'm good with the content in the update.

stephenmcgruer commented 3 years ago

LGTM. Will merge on July 24th (one week after latest update).