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

Fix headless test runner on macOS High Sierra #1298

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

LegNeato
Copy link
Contributor

No description provided.

@alexcrichton
Copy link
Contributor

Thanks for this! Can you detail a bit about what the bug was? I'm not entirely sure what #[serde(flatten)] is doing here but it seems like it's fixing the issue?

@LegNeato
Copy link
Contributor Author

LegNeato commented Feb 27, 2019

Sure! Apparently post High Sierra the format of safaridriver's response changed. On my machine (High Sierra), I get:

{
  "status": 0,
  "sessionId": "7E00E58A-859E-4127-9E0A-F67FBE4CB647",
  "value": {
    "version": "13605.3.8",
    "rotatable": false,
    "applicationCacheEnabled": true,
    "databaseEnabled": true,
    "handlesAlerts": true,
    "cleanSession": true,
    "browserName": "safari",
    "javascriptEnabled": true,
    "locationContextEnabled": false,
    "platform": "macOS",
    "webStorageEnabled": true,
    "cssSelectorsEnabled": true,
    "nativeEvents": true,
    "platformName": "macOS"
  }
}

Because Option::or is eager, for the case where a value entry exists but doesn't contain sessionId, deserializing would fail.

serde(flatten) is supposed to hoist the keys of the substruct into the parent struct, but after manually testing I see it doesn't behave the way I want for this fix. So I pushed a new change that should work for all cases.

In this playground link, you can see the three cases:

  • sessionId is at the top level with a value entry with no sessionId ("--legacy" or High Sierra and below)
  • sessionId is at the top level without a value entry
  • no sessionId at the top level but one in value

@alexcrichton
Copy link
Contributor

Ah I see, thanks for the explanation! The PR as-is definitely makes sense to me as well!

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.

2 participants