Skip to content

Add estimate parameter to update_virtual_pressure_source #51194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 21, 2025
Merged

Add estimate parameter to update_virtual_pressure_source #51194

merged 6 commits into from
Mar 21, 2025

Conversation

arskama
Copy link
Contributor

@arskama arskama commented Mar 7, 2025

spec PR: w3c/compute-pressure#305

This PR add own pressure estimate parameter to the update_virtual_pressure_source command as specified in https://w3c.github.io/compute-pressure/?experimental=1#update-virtual-pressure-source

@arskama arskama requested a review from a team as a code owner March 7, 2025 14:22
@wpt-pr-bot wpt-pr-bot added infra testdriver.js wptrunner The automated test runner, commonly called through ./wpt run labels Mar 7, 2025
@arskama
Copy link
Contributor Author

arskama commented Mar 11, 2025

@foolip ptal.

@past past removed the request for review from DanielRyanSmith March 11, 2025 16:28
@arskama
Copy link
Contributor Author

arskama commented Mar 12, 2025

@past is there anything wrong with the patch, so that people do not review it?

You ve been reviewing a similar patch earlier #48035, therefore I wanted to know (since you re active) if you could review this one too. Thanks

@past
Copy link
Member

past commented Mar 12, 2025

I think it's just that people are busy and haven't gotten around to reviewing this yet. I will try to get to it before the end of the week if nobody beats me to it.

Have you looked at the infrastructure test failures?

@arskama
Copy link
Contributor Author

arskama commented Mar 13, 2025 •
edited
Loading

@past I don't see any obvious correlation between the infrastructure test failures and my patch.
I m just wondering if it is because of the default value estimate = -1.0

But I've tested this patch with my upcoming changes on Chromium and it seems to work as intended.
run_wpt_test.py for both headless_shell and chrome are successful.

I noticed that many other PR are failing on exactly the same infra tests.

arskama added 4 commits March 13, 2025 19:39
@arskama
Add estimate parameter to update_virtual_pressure_source
e0e3ced 
spec PR: w3c/compute-pressure#305

This PR add own pressure estimate parameter to the
update_virtual_pressure_source command as specified in
https://w3c.github.io/compute-pressure/?experimental=1#update-virtual-pressure-source
@arskama
Fix wrong update between sensor and pressure source
20ca2bc 
There was a clear mistake in updating update_virtual_sensor
instead of update_virtual_pressure_source in testdriver-extra.js
@arskama
remove unnecessary default value
5f7fa26
@arskama
fix closing bracket
455d6bf
@arskama arskama force-pushed the ownContributionEstimate branch from d855392 to 455d6bf Compare March 13, 2025 17:48
@arskama
Copy link
Contributor Author

arskama commented Mar 17, 2025

@foolip @jgraham can you place have a look? Thanks

@arskama
Copy link
Contributor Author

arskama commented Mar 20, 2025

I think it's just that people are busy and haven't gotten around to reviewing this yet. I will try to get to it before the end of the week if nobody beats me to it.

Have you looked at the infrastructure test failures?

#51306 has exactly the same amount of tests and same failures.

@arskama
Copy link
Contributor Author

arskama commented Mar 20, 2025

I think it's just that people are busy and haven't gotten around to reviewing this yet. I will try to get to it before the end of the week if nobody beats me to it.

Have you looked at the infrastructure test failures?

@past apparently nobody beat you to it. :)

past
past approved these changes Mar 20, 2025
View reviewed changes
tools/wptrunner/wptrunner/testdriver-extra.js Outdated
@@ -468,8 +468,8 @@
return create_context_action("create_virtual_pressure_source", context, {source_type, metadata});
};

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});
window.test_driver_internal.update_virtual_pressure_source = function(source_type, sample, estimate = -1.0, 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.

Is -1.0 the right default value here? I couldn't tell from the spec why that would be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree! I removed it and checked it still works when rolling out in chromium.

Thanks

@past
Copy link
Member

past commented Mar 20, 2025

I think it's just that people are busy and haven't gotten around to reviewing this yet. I will try to get to it before the end of the week if nobody beats me to it.
Have you looked at the infrastructure test failures?

#51306 has exactly the same amount of tests and same failures.

I agree that the infrastructure test failures are unrelated to this PR.

@past past merged commit 0e07e38 into web-platform-tests:master Mar 21, 2025
31 of 33 checks passed
mohamedamir pushed a commit to mohamedamir/wpt that referenced this pull request Mar 27, 2025
@arskama @mohamedamir
Add estimate parameter to update_virtual_pressure_source (web-platfor…
a8e1a56 
…m-tests#51194)

* Add estimate parameter to update_virtual_pressure_source

spec PR: w3c/compute-pressure#305

This PR add own pressure estimate parameter to the
update_virtual_pressure_source command as specified in
https://w3c.github.io/compute-pressure/?experimental=1#update-virtual-pressure-source

* Fix wrong update between sensor and pressure source

There was a clear mistake in updating update_virtual_sensor
instead of update_virtual_pressure_source in testdriver-extra.js

* remove unnecessary default value

* fix closing bracket

* Fix wrong indent

* remove default value for estimate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@past past past approved these changes

@foolip foolip Awaiting requested review from foolip

@jgraham jgraham Awaiting requested review from jgraham

Assignees

@foolip foolip

Labels
infra testdriver.js wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arskama @past @foolip @wpt-pr-bot