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

Test optional resource load attributes #236

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

gingerbenw
Copy link
Member

@gingerbenw gingerbenw commented Jun 27, 2023

Goal

HTTP status code and body size attributes (encoded and decoded) have patchy browser coverage, and by adding new step that runs any given step depending on browser version, we can assert these attributes are correct on the supported browsers

@gingerbenw
Copy link
Member Author

I'm open to other options, but this seemed to make sense to me

@github-actions
Copy link

github-actions bot commented Jun 27, 2023

Browser bundle size

NPM build

Package
Before 183.70 kB
After 183.70 kB
± No change

CDN build

Unminified Minfied Minified + gzipped
Before 82.89 kB 31.14 kB 9.60 kB
After 82.89 kB 31.14 kB 9.60 kB
± No change No change No change

Code coverage

Coverage values did not change👌.

Total:

Lines Branches Functions Statements
92.82%(+0%) 88.47%(+0%) 94.37%(+0%) 92.68%(+0%)

Generated against 6661fb4 on 1 September 2023 at 15:24:01 UTC

@imjoehaines
Copy link
Contributor

Could we assert these are present on browsers that should definitely support them, in the same way we do other browser support things? i.e. rather than if present… it'd be if supported…

Base automatically changed from PLAT-10231/resource-load-sub-spans to next June 27, 2023 12:02
@gingerbenw gingerbenw force-pushed the PLAT-10231/response-content-length branch 2 times, most recently from a96a7e7 to 9e1aebc Compare June 27, 2023 17:04
@gingerbenw
Copy link
Member Author

Could we assert these are present on browsers that should definitely support them, in the same way we do other browser support things? i.e. rather than if present… it'd be if supported…

Actually followed a similar pattern to the flutter notifier, and created a step that checks browser version before running a given step. only improvement now would be to show the step was skipped (in blue) if it's not in the supported list

@gingerbenw gingerbenw marked this pull request as ready for review June 29, 2023 14:35
@gingerbenw gingerbenw requested a review from lemnik June 29, 2023 14:35

# Image status code and body size have patchy browser coverage
And on the browser Chrome 109: the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.status_code" equals 200
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads a lot like "on Chrome 109 and only Chrome 109 …" to me

In bugsnag-laravel there's a step that lets you use operators so this could be:

And on Chrome versions >= 109:
  """
  the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.status_code" equals 200
  """

Would a similar thing make sense here?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's quite a nice pattern, think we could make it work for Chrome versions >= 109, Firefox versions >= 76, Safari >= 16 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this should do it:

Then(/^on ((?:[A-z]+ versions (?:>=?|<=?|==) [0-9.]+(?:, )?)+):$/) do |browser_specs, steps_to_run|
  spec_matcher = /^([A-z]+) versions (>=?|<=?|==) ([0-9.]+)$/

  browser_specs.split(", ").each do |browser_spec|
    browser_spec.scan(spec_matcher) do |name, operator, version|
      should_run_steps = $browser.name.casecmp?(name) && $browser.version.send(operator, version)

      # make sure this step is debuggable!
      $logger.debug("#{$browser.name} == #{name} && v#{$browser.version} #{operator} #{version}? #{should_run_steps}")

      if should_run_steps
        steps_to_run.each_line(chomp: true) do |step_to_run|
          step(step_to_run)
        end
      else
        indent = " " * 4
        # e.g. "a step\nanother step\n" -> "    1) a step\n    2) another step"
        steps_indented = steps_to_run.each_line.map.with_index(1) { |step, i| "#{indent}#{i}) #{step.chomp}" }.join("\n")

        $logger.info("Skipping steps on #{$browser.name} v#{$browser.version}:\n#{steps_indented}")
      end
    end
  end
end

@gingerbenw gingerbenw force-pushed the PLAT-10231/response-content-length branch 2 times, most recently from c4751bd to 0974e0a Compare July 13, 2023 10:18
@@ -195,6 +195,31 @@
end
end

Then(/^on ((?:[A-z]+ versions (?:>=?|<=?|==) [0-9.]+(?:, )?)+):$/) do |browser_specs, steps_to_run|
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't working:

[10:23:59] INFO: Skipping steps on chrome vInfinity:
1) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length" equals 2202
2) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length_uncompressed" equals 2202
[10:23:59] INFO: Skipping steps on chrome vInfinity:
1) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length" equals 2202
2) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length_uncompressed" equals 2202
[10:23:59] INFO: Skipping steps on chrome vInfinity:
1) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length" equals 2202
2) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length_uncompressed" equals 2202
[10:23:59] INFO: Skipping steps on chrome vInfinity:
1) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length" equals 2202
2) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length_uncompressed" equals 2202
[10:23:59] INFO: Skipping steps on chrome vInfinity:
1) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length" equals 2202
2) the trace payload field "resourceSpans.0.scopeSpans.0.spans.1" integer attribute "http.response_content_length_uncompressed" equals 2202

@gingerbenw gingerbenw force-pushed the PLAT-10231/response-content-length branch from dee3cfa to 4cc5644 Compare July 14, 2023 13:03
@gingerbenw gingerbenw force-pushed the PLAT-10231/response-content-length branch from 4cc5644 to 6661fb4 Compare September 1, 2023 15:22
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