Skip to content

Fix the third crash in the chrome user suite test by remembering to pass excludeThisKeyword #33711

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

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Oct 1, 2019

#33687 only fixed one of two issues, as it happens.

In #33537, isBindableObjectDefinePropertyCall was changed to use a new helper to handle element accesses, but Object.defineProperty codepaths don't have handling for binding this members (and didn't handle them before), so Object.definteProperty(this._whatever, "field", { value: 12 }) would cause a crash. 🙁

It looks we we just forgot to pass in the excludeThisKeyword parameter at this callsite.

This time I've validated the that the whole chrome-devtools-frontend test no longer crashes (and, in fact, has way fewer errors)~

@weswigham weswigham requested a review from andrewbranch October 1, 2019 19:57
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Good catch, thanks 🙌

@weswigham
Copy link
Member Author

@DanielRosenwasser @RyanCavanaugh

Should we chalk this crash up as a known issue in the beta? 😦

@weswigham weswigham merged commit 410ff90 into microsoft:master Oct 1, 2019
@weswigham weswigham deleted the fix-user-suite-crash-again branch October 1, 2019 20:14
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 1, 2019

I don't think this is a beta blocker (it's already out), and I don't think it's bringing up in the release notes.

I think it is worth calling out that if this was due to a lack of nightly test coverage because everything was merged in yesterday, it's the reason we should have pushed the beta out by at least another day. Even if not, that seems like a good precedent to me.

@weswigham
Copy link
Member Author

No, this was hit in the user suite run overnight - that's why it got fixed so quickly - it's just we did the first merge of most of this on Friday, fixed a pair of crashes on Monday, ran again overnight on Monday, then realized there as still a third crash in the same test, which this fixes. The failure is simply assuming fixing the isolated, minimized repro fixes the entire user test. If you're OK with shipping a beta with failing extended tests... I guess that's OK; but maybe passing/accepted user+rwc+dt tests are part of our release criteria nowadays, no? (It's not done automatically, but that's probably a good idea...)

@weswigham
Copy link
Member Author

I've just enabled rolling rwc builds on release-* branches (which, um, might be awkward because baselines between release branches and master are shared, due to the internal repro shenanigans - might disable that again later), and rolling user-type runs on both master and release-* branches (and I'll remove the nightly run once that's working). Rolling DT runs will follow ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants