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

Update reftests in manifest when reftests become non-reftests #9598

Merged
merged 3 commits into from Feb 21, 2018

Conversation

Projects
None yet
5 participants
@foolip
Copy link
Contributor

foolip commented Feb 21, 2018

In #9523 some tests were
converted from reftests to testharness, but the incremental manifest
update mechanism doesn't handle this case. This appears to be the cause
of the mysterious Travis failures on that PR.


This change is Reviewable

Update reftests in manifest when reftests become non-reftests
In #9523 some tests were
converted from reftests to testharness, but the incremental manifest
update mechanism doesn't handle this case. This appears to be the cause
of the mysterious Travis failures on that PR.
@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 21, 2018

Build PASSED

Started: 2018-02-21 12:41:25
Finished: 2018-02-21 12:56:06

View more information about this build on:

@foolip foolip merged commit 8619507 into master Feb 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@foolip foolip deleted the manifest-reftest-changes branch Feb 21, 2018

@@ -91,6 +91,8 @@ def update(self, tree):
hash_changed = True
else:
new_type, manifest_items = old_type, self._data[old_type][rel_path]
if old_type == "reftest" and new_type != old_type:

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 21, 2018

Contributor

This might need to include "reftest_node" too?

This comment has been minimized.

Copy link
@foolip

foolip Feb 21, 2018

Author Contributor

I actually don't quite understand what "reftest_node" is. Does it contain all of files involved in a reftest, everything that's not used a ref, or something else? What kind of renaming would one have to do to run into a problem here?

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 21, 2018

Contributor

If we had:

a.html: <link rel=match href=b.html>
b.html: <link rel=match href=c.html>
c.html: hello world

Then:

a.html would be a reftest (it's a test we want to run)
b.html would be a reftest_node (it's in the reftest graph and can itself be run)
c.html would be a support (because we can't tell anything about it statically)

@jgraham I think that's right?

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 21, 2018

Contributor

Yeah. I guess it's like theoretically possible you have A==B==C and you update B to be a testharness test but still use it as the ref for A, but that seems pretty unlikely, so this case might not get hit in practice, so it's a bit of an edge case correctness issue. File a bug and mark it "good first bug".

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 26, 2018

Contributor

I think a less hypothetical case is if b.html changes to support? Filing an issue now.

This comment has been minimized.

Copy link
@gsnedders
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.