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: handle multiple strings that look like they might be JSON #361

Merged
merged 2 commits into from
May 7, 2024

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented May 5, 2024

The test output can have occurrences of { that aren't the string we are looking for, eg:

dialer-1    | test browser
dialer-1    | [JavaScript Error: "Clear-Site-Data header found. Unknown value “"cache"”." {file: "http://127.0.0.1:34451/" line: 0}]
dialer-1    | [JavaScript Error: "Clear-Site-Data header forced the clean up of “cookies” data." {file: "http://127.0.0.1:34451/" line: 0}]
dialer-1    | [JavaScript Error: "Clear-Site-Data header forced the clean up of “storage” data." {file: "http://127.0.0.1:34451/" line: 0}]

These fail to parse so improve the results parsing by:

  1. Try parsing every instance of {...} in the output, not just the first
  2. Test the output for the handshakePlusOneRTTMillis and pingRTTMilllis keys

The test output can have multiple instances of strings that contain
`{`, eg:

```
dialer-1    | test browser
dialer-1    | [JavaScript Error: "Clear-Site-Data header found. Unknown value “"cache"”." {file: "http://127.0.0.1:34451/" line: 0}]
dialer-1    | [JavaScript Error: "Clear-Site-Data header forced the clean up of “cookies” data." {file: "http://127.0.0.1:34451/" line: 0}]
dialer-1    | [JavaScript Error: "Clear-Site-Data header forced the clean up of “storage” data." {file: "http://127.0.0.1:34451/" line: 0}]
```

These fail to parse so improve the results parsing by:

1. Try to find a matching closing `}` for every `{`
2. Test the output for the `handshakePlusOneRTTMillis` and `pingRTTMilllis` keys
3. Try parsing every instance of `{...}` in the output, not just the first
@achingbrain achingbrain requested a review from MarcoPolo May 5, 2024 13:18
@MarcoPolo
Copy link
Contributor

Why is this on stdout? Shouldn’t these logs be stderr?

@achingbrain
Copy link
Member Author

achingbrain commented May 5, 2024

I’m not sure why it’s on stdout, I agree yes it should be on stderr, but at the same time the function that parses the output should be more robust.

@achingbrain
Copy link
Member Author

achingbrain commented May 7, 2024

There's a fix for this specific issue at hugomrdias/playwright-test#661 but I'm going to merge this because making the detection more robust will help us in the future.

Browsers do sometimes just log uncatchable errors (e.g. https://issues.chromium.org/issues/40069954) which will break this again without the fix here if the message happens to have a { character in it.

We might want to think of a way to transmit results in a less fragile way but that can be done as a follow up if it's desirable.

@achingbrain achingbrain merged commit fd59ed7 into master May 7, 2024
2 checks passed
@achingbrain achingbrain deleted the fix/handle-mutliple-json-looking-strings branch May 7, 2024 10:10
@MarcoPolo
Copy link
Contributor

We might want to think of a way to transmit results in a less fragile way but that can be done as a follow up if it's desirable.

Reserving stdout for the test results and stderr for everything else (as is convention) would work just fine. My guess is that the test framework we're using just dumps everything to stdout and doesn't distinguish.

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