Add Maze Runner end to end test suite #351

bengourley · 2018-05-18T15:55:43Z

Goal

This PR adds a new end-to-end test suite using maze-runner. This tool factors out common tasks and assertion utilities that are needed across our notifier libraries. The main goals are:

more complete coverage
commonality amongst notifier testing
facilitate simple addition of scenarios as they arise

Changeset

Added

The entire set of tests, fixtures and support scripts are added in the features directory, which is a cucumber/gherkin convention.

Additionally a Gemfile and Gemfile.lock are added to describe the ruby dependencies.

Tests were added for each of the following cases:

an app containing bundled bugsnag-js using webpack
a html page linking dist/bugsnag.js
a library containing bundled bugsnag-js using rollup
Syntax errors
Thrown exceptions
Malformed URIs
Unhandled promise rejections
Invoking an undefined function
Thrown exception with a malformed stacktrace (TODO)
Invoking notify from try/catch
Invoking notify from a Promise.catch()
Modifying report contents from a per-report callback
Setting release stage
Setting user info
Redacting IP address
Global callbacks
Disabling autoNotify

Removed

The e2e directory stands to be removed as it is made redundant by the new tests. At the time of writing, coverage would decrease on the following scenarios, so they need to be addressed with maze tests before e2e can sensibly be removed:

Changed

Travis configuration has been updated and currently only runs the maze tests. There is now a build matrix, running a job for each browser to go through all of the end to end scenerios.

The existing integration tests browser/**/*.test.js will need to be re-added to the travis build. The test configuration may need to be updated slightly to run this in each browser ci job.
The existing unit tests base/**/*.test.js will need to be added re-added to the travis build. This could be done with travis's new build stages because it needn't run for every browser.

Review

For the submitter, initial self-review:

Commented on code changes inline explain the reasoning behind the approach
Reviewed the test cases added for completeness and possible points for discussion
A changelog entry was added for the goal of this pull request
Check the scope of the changeset - is everything in the diff required for the pull request?
This pull request is ready for:
- Initial review of the intended approach, not yet feature complete
- Structural review of the classes, functions, and properties modified
- Final review

For the pull request reviewer(s), this changeset has been reviewed for:

Consistency across platforms for structures or concepts added or modified
Consistency between the changeset and the goal stated above
Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
Usage friction - is the proposed change in usage cumbersome or complicated?
Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
Concurrency concerns - if components are accessed asynchronously, what issues will arise
Thoroughness of added tests and any missing edge cases
Idiomatic use of the language

This commit provides the test architecture and setup. More scenarios to follow.

fractalwrench · 2018-05-18T16:20:14Z

Is there any way we could split this PR up at all into logical sections? I can have a look through, but I'm not confident that I'd see everything that needs catching in a 2.5k+ changeset.

fractalwrench · 2018-05-18T16:22:00Z

features/before_send.feature

+  And the test should run in this browser
+  And I let the test page run for up to 10 seconds
+  And I wait for 5 seconds
+  Then I should receive 1 request


And the test should run in this browser And I let the test page run for up to 10 seconds And I wait for 5 seconds Then I should receive 1 request

This seems to be a relatively common pattern - would it be worth creating a step which shortens this? On Android/Cocoa we have something along the lines of "When I run the app with the defaults for X"

Sounds like a good idea.

I was also considering removing the "And the test should run in this browser" from all the tests I know I don't need that check. Lots of the fixtures harcode "YES" for this purpose, which is pretty redundant.

"should run in this browser" seems like a nice general check, though my one concern with it would be accidentally excluding tests. Though given the implementation, that seems unlikely. Maybe the "YES" could be assumed unless the value is otherwise set?

fractalwrench · 2018-05-18T16:23:21Z

features/before_send.feature

+Feature: beforeSend callbacks
+
+Scenario Outline: modifying report via beforeSend in config
+  When I navigate to the URL "/before_send/<type>/a.html"


What does <type> stand for?

it's a placeholder for this "Scenario Outline". That means that this scenario runs once for every item in the table below it. The <type> placeholder is replaced by a different value from the table each time.

fractalwrench · 2018-05-18T16:26:39Z

features/lib/fast-selenium.rb

@@ -0,0 +1,19 @@
+# copy pasta from https://www.browserstack.com/automate/ruby


Are there any licensing issues around this? It's probably fine, but IANAL

Doubtful since it came right out of the docs

bengourley · 2018-05-21T08:54:36Z

Is there any way we could split this PR up at all into logical sections?

There are individual commits for each set of features that are tested. There is a view in the PR interface where you can look at one commit at a time, e.g: ac9db8c

fractalwrench · 2018-05-21T09:05:51Z

.npmignore

@@ -1,2 +1,3 @@
 coverage
 .nyc_output
+features


Should this also include entries in the .gitignore? i.e. maze_output, others?

I believe .npmignore inherited rules from .gitignore (so you only need to specify the differences) but I will confirm.

fractalwrench · 2018-05-21T09:08:12Z

.travis.yml

  browserstack:
    username: bugsnagplatforms1
    access_key:
     secure: "lB2aZc0HzuinyhSJyP98lzDJTbZsroEOJTrS/flnLLsHb/C096zjhwkWexHnfmNIPaY5SB2FF0G6zcnknlmsXg5TgxQiK7bdyJmkcAetGi+cSKnGaBq7GZXVhbWh++MRnddQDGV6jrSr7aqrWf85ncq3L4//9BTV6uAhDBWu2bo="
-before_script:
- curl -L https://codeclimate.com/downloads/test-reporter/test-reporter-latest-linux-amd64


Has this been replaced elsewhere?

No, it needs to be readded as per the "changed" section in the PR body (those two sets of tests are what generate the coverage).

fractalwrench · 2018-05-21T09:10:43Z

features/fixtures/handled/browserify/a.html

+<!DOCTYPE html>
+<html>
+  <head>
+    <script src="dist/a.js"></script>


What do a/b/c stand for in the JS + HTML files - is there a more descriptive naming scheme that would make sense to use?

fractalwrench · 2018-05-21T09:14:25Z

features/fixtures/handled/browserify/src/a.js

+    setTimeout(function () {
+      var el = document.getElementById('bugsnag-test-state')
+      el.textContent = el.innerText = 'DONE'
+    }, 5000)


It would be good to extract the 5000 from these scripts to one place, in case we decide to change it in future. Is this the delay required to send a report to Bugsnag?

fractalwrench · 2018-05-21T09:16:18Z

features/fixtures/handled/browserify/src/b.js

+      setTimeout(function () {
+        var el = document.getElementById('bugsnag-test-state')
+        el.textContent = el.innerText = 'DONE'
+      }, 5000)


This beforeSend function appears to be the same in a few files. Is there any way of extracting this code to one location?

Having it duplicated means that each e2e test fixture is self-contained, making it easier to follow and and reduces the chance that a slight change in a helper library could break/invalidate the test result.

fractalwrench · 2018-05-21T09:23:27Z

features/lib/server.rb

+
+root = File.expand_path File.join(__dir__, '..', 'fixtures')
+server = WEBrick::HTTPServer.new :Port => ENV['PORT'], :DocumentRoot => root
+server.start


What does this file serve?

All of the static assets in features/fixtures. This expression is resolving the path to that directory in a cross platform way:

File.expand_path File.join(__dir__, '..', 'fixtures')

fractalwrench · 2018-05-21T09:25:52Z

features/steps/browser_steps.rb

+  }
+end
+
+Then(/^the request is a valid browser payload for the error reporting API$/) do


The checks here are good, was there a reason why this needs to be separate from the standard error reporting API step in mazerunner? There may be other notifiers which don't send an API key in the headers.

fractalwrench · 2018-05-21T09:27:18Z

features/support/env.rb

+
+def current_ip
+  # Parses the output of `ifconfig` to retreive the host IP for docker to talk to
+  # Breaks compatability with Windows


Is there any simple way we can maintain compatibility with Windows here?

fractalwrench · 2018-05-21T09:30:22Z

features/unhandled_errors.feature

+  Then I should receive 1 request
+  And the request is a valid browser payload for the error reporting API
+  And the exception matches the "unhandled_syntax" values for the current browser
+  # TODO: add stacktrace assertions to this type of error


Does this still need to be addressed?

fractalwrench · 2018-05-21T09:32:52Z

features/release_stage.feature

@@ -0,0 +1,48 @@
+@release_stage
+Feature: Configuring releaseStage and notifyReleaseStages


It would be good to add scenarios to cover nil/empty releaseStages as well

fractalwrench · 2018-05-21T09:36:48Z

features/fixtures/ip_redaction/script/a.html

+  <head>
+    <script src="/node_modules/bugsnag-js/dist/bugsnag.min.js"></script>
+    <script type="text/javascript">
+      var ENDPOINT = decodeURIComponent(window.location.search.match(/ENDPOINT=(.+)/)[1])


I think I've seen window.location.search.match(/ENDPOINT=(.+)/)[1] (and a couple of similar snippets) in multiple places within the fixture scripts. Is it sensible to extract these sort of snippets into separate scripts, or is that a bad idea?

fractalwrench · 2018-05-21T09:43:55Z

Added a few more inline comments. It'd be good to get someone else to also have a look through this, given the scope of the changes.

kattrali

This looks good. I went over the features and helpers in some detail though only sampled the fixtures due to the size. Left a few comments on the existing conversations for thought.

bengourley added 7 commits May 16, 2018 12:28

test(e2e): Switch to maze-runner based e2e tests

ad65b8b

This commit provides the test architecture and setup. More scenarios to follow.

test(e2e): Add scenarios for unhandled errors

0ddee28

test(e2e): Add scenarios for releaseStage configuration

ac9db8c

test(e2e): Add scenarios for user info configuration

7e3f814

test(e2e): Add scenarios for beforeSend callbacks

f671ed9

test(e2e): Add scenarios for redacting IP addresses

0166beb

test(e2e): Add scenarios for autoNotify configuration option

f6b44b9

bengourley changed the title ~~Maze end to end test suite~~ Add Maze Runner end to end test suite May 18, 2018

bengourley requested a review from a team May 18, 2018 16:08

$fractalwrench$

fractalwrench reviewed May 18, 2018

View reviewed changes

$fractalwrench$

fractalwrench reviewed May 21, 2018

View reviewed changes

kattrali previously approved these changes May 22, 2018

View reviewed changes

ci(codeclimate): Remove offending (unused) code and TODO

5dfb505

bengourley dismissed kattrali’s stale review via 5dfb505 May 22, 2018 14:52

ci(codeclimate): Ignore test fixtures

88f439c

bengourley added 7 commits May 23, 2018 16:07

build(deps): Remove unused dependencies

28884cc

test(e2e): Add maze-flavoured typescript test

118b356

test(e2e): Add scenario for bluebird promise compatibility

f341479

test(e2e): Add maze scenario for existing inline script/navigation

3b8945f

chore(npmignore): Prevent inclusion of unnecessary files in npm package

caba79e

test(e2e): Remove obsolete test suite

b56027e

ci(travis): Use build stages to run all sets of tests

b13d877

kattrali approved these changes May 24, 2018

View reviewed changes

kattrali merged commit 242812b into master May 24, 2018

kattrali deleted the maze-end-to-end-test-suite branch May 24, 2018 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Maze Runner end to end test suite #351

Add Maze Runner end to end test suite #351

bengourley commented May 18, 2018 •

edited

fractalwrench commented May 18, 2018

$@fractalwrench$ fractalwrench May 18, 2018 •

edited

bengourley May 21, 2018

kattrali May 22, 2018 •

edited

$@fractalwrench$ fractalwrench May 18, 2018

bengourley May 21, 2018

$@fractalwrench$ fractalwrench May 18, 2018

kattrali May 19, 2018

bengourley commented May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

bengourley May 21, 2018 •

edited

$@fractalwrench$ fractalwrench May 21, 2018

bengourley May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

kattrali May 22, 2018

$@fractalwrench$ fractalwrench May 21, 2018

bengourley May 21, 2018 •

edited

$@fractalwrench$ fractalwrench May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

$@fractalwrench$ fractalwrench May 21, 2018

fractalwrench commented May 21, 2018

kattrali left a comment

		@@ -0,0 +1,19 @@
		# copy pasta from https://www.browserstack.com/automate/ruby

		@@ -0,0 +1,48 @@
		@release_stage
		Feature: Configuring releaseStage and notifyReleaseStages

Add Maze Runner end to end test suite #351

Add Maze Runner end to end test suite #351

Conversation

bengourley commented May 18, 2018 • edited

Goal

Changeset

Added

Removed

Changed

Review

fractalwrench commented May 18, 2018

fractalwrench May 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kattrali May 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengourley commented May 21, 2018

Choose a reason for hiding this comment

bengourley May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengourley May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented May 21, 2018

kattrali left a comment

Choose a reason for hiding this comment

bengourley commented May 18, 2018 •

edited

$@fractalwrench$ fractalwrench May 18, 2018 •

edited

kattrali May 22, 2018 •

edited

bengourley May 21, 2018 •

edited

bengourley May 21, 2018 •

edited