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

Add compute pressure-related commands to testdriver #48035

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

JuhaVainio
Copy link
Contributor

Spec PR: w3c/compute-pressure#284

This PR adds the required infrastructure to manipulate compute pressure from testdriver. The three new commands correspond to the three WebDriver extension commands added by the spec PR above.

Spec PR: w3c/compute-pressure#284

This PR adds the required infrastructure to manipulate compute pressure
from testdriver. The three new commands correspond to the three
WebDriver extension commands added by the spec PR above.
@JuhaVainio JuhaVainio requested a review from a team as a code owner September 9, 2024 08:38
@wpt-pr-bot wpt-pr-bot added docs infra testdriver.js wptrunner The automated test runner, commonly called through ./wpt run labels Sep 9, 2024
@JuhaVainio
Copy link
Contributor Author

@foolip @past : Can you review this. Thank you.

* @param {String} source_type - A `virtual pressure source type
* <https://w3c.github.io/compute-pressure/#dom-pressuresource>`_
* such as "cpu".
* @param {String} sample - A `virtual pressure sample
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the spec, I think you meant "pressure state" instead of "pressure sample" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes in this reference the text talking about the spec should be changed and is changed in this commit 7cc8325

But the spec defines the parameter name as "sample" in Update virtual pressure source -chapter.

Also spec defines "sample" in sentence like "A pressure source
has an associated latest sample
".

And in one step of "Update virtual pressure source" service it says
"7. Let sample be the result of invoking get a property "sample" from parameters."

* virtual pressure source of the given type does not
* exist).
*/
update_virtual_pressure_source: function(source_type, sample, context=null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sample -> state (or pressure_state)

throw new Error("create_virtual_pressure_source() is not implemented by testdriver-vendor.js");
},

async update_virtual_pressure_source(source_type, sample, context=null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/sample/state/

def __call__(self, payload):
source_type = payload["source_type"]
sample = payload["sample"]
return self.protocol.pressure.update_virtual_pressure_source(source_type, sample)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

return self.webdriver.send_session_command("POST", "pressuresource", body)

def update_virtual_pressure_source(self, source_type, sample):
body = {"sample": sample}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

pass

@abstractmethod
def update_virtual_pressure_source(self, source_type, sample):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

};

window.test_driver_internal.update_virtual_pressure_source = function(source_type, sample, context=null) {
return create_context_action("update_virtual_pressure_source", context, {source_type, sample});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And finally here.

@JuhaVainio
Copy link
Contributor Author

@past Could you check my reply in https://github.com/web-platform-tests/wpt/pull/48035/files/ba89a9af005235fed37148191de4c22ef0ade281#r1753802603

I fixed the document related text in code and explain why the actual variable is named as it is.

@past
Copy link
Member

past commented Sep 16, 2024

Thanks for the explanation, this seems fine to me then.

@past past merged commit 384f5d9 into web-platform-tests:master Sep 16, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs infra testdriver.js wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants