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

Support errorsummary files in update-wpt #22230

Closed
wants to merge 1 commit into from

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Nov 20, 2018

These are shorter and easier to deal with. We filter intermittents through these, so it's convenient if WPT can directly accept these files.

The current support is a bit hacky since the errorsummary doesn't save run-info. An alternate way to do this would be to modify the errorsummary printer to use the standard format, just pared down.

r? @jgraham


This change is Reviewable

These are shorter and easier to deal with. We filter intermittents
through these, so it's convenient if WPT can directly accept these
files.

`
@servo-wpt-sync
Copy link
Collaborator

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14138.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

I don't know. I think this is broken in a couple of related ways. I"m a bit surprised that not provising run_info works; I feel like it should break if any results change that already have conditional results because we try to evaluate the existing condition but don't have any input data. And, related to that, since errorsummary files by definition only contain failures, this can't ever handle the case where there are different results in different configurations (even with runinfo files).

So, I can see that this will be faster if you happen to fall into the subset of cases it supports. But I wonder if using wptreport.json files is enough (these are already much faster than using the raw log files).

self.run_info = run_info_intern.store({})

test_data.set(test_id, subtest, "status", self.run_info, result)
print subtest, result
Copy link
Contributor

Choose a reason for hiding this comment

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

Debugging code?

@Manishearth
Copy link
Member Author

We could emit run_info in errorsummary files.

And, related to that, since errorsummary files by definition only contain failures, this can't ever handle the case where there are different results in different configurations (even with runinfo files).

We could restrict this to the common case -- when you run update-wpt foo.log with a single log file.

We also filter errorsummary files for intermittents (this can't be done with wptreport.json)

@jgraham
Copy link
Contributor

jgraham commented Nov 22, 2018

We could emit run_info in errorsummary files.

Sure, although there's a nonzero chance that will break treeherder; the errorsummary parsing there has previously been a little fragile. I think doing that would be a prerequisite to landing this.

And, related to that, since errorsummary files by definition only contain failures, this can't ever handle the case where there are different results in different configurations (even with runinfo files).

We could restrict this to the common case -- when you run update-wpt foo.log with a single log file.

Yeah, so I think I would be happy to land this if we get real runinfo and if we check that there's only a single input file (that very much isn't the common case for me, but maybe it's different for others).

We also filter errorsummary files for intermittents (this can't be done with wptreport.json)

Fixing that somehow seems like a interesting idea. Like, maybe instead of disabling tests we should allow setting status=FLAKY and then recording multiple allowed statuses or something. Then you could dispense with the filtering altogehter and just mark known flaky tests in the ini files vs having a servo-specific intermittent-filtering pass.

@Manishearth
Copy link
Member Author

Closing for now, I'd like to see this fixed eventually but probably need to approach it differently.

@Manishearth Manishearth deleted the wptupdate-errorsummary branch July 11, 2019 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants