Integration Test Refactor #131

jshcrowthe · 2017-08-13T05:55:34Z

This is a wholesale refactor of the integration test harnesses to better isolate them from the rest of the SDK testing.

Each suite can leverage the files found in integration/shared which is a spec file of the SDK (built from the existing specfiles) and a simple validator. I then run a suite of tests that validates the SDK in each of its consumption mechanisms. We can add more as needed (e.g. reactNative, cordova) and as each directory's tests are run by hand (i'm currently providing a runner.sh file for the existing tests) it can be used to simulate whatever environment.

Once Travis CI supports build matrices in secondary jobs the CI for this will get even simpler. Until then we have a little manual work which isn't that bad as I don't really want to have an overabundance of fragile Integration testing.

I will flesh out the README.md to detail the specfile (i.e. integration/shared/namespaceDefinition.json and how the integration tests are to be written)

WIP: fiddling with Travis YML Jobs WIP: refactor build matrix WIP: remove integration tests (Will reimpl) WIP: remove files that snuck in WIP: remove unneeded arg WIP: add node_modules bin to path fix(*): remove project config (intended to be supplied) WIP: reset TEST_ENV for int tests WIP: update Travis yarn dep WIP: use before_install script instead of install script

WIP: reimplement browserify/webpack integration tests WIP: fiddling with int test CI WIP: build matrix for Integration tests (parallelize those jobs too) WIP: trying to parallelize int tests WIP: remove unneeded env assignment WIP: blahhh... WIP: should work... WIP: removing env WIP: removing test harness I haven't started yet test(typescript): add typescript namespace tests

jshcrowthe · 2017-08-14T16:10:33Z

@sphippen I discovered an implicit dependency in the storage codebase that I addressed here as it was breaking tests when not all tested together. PTAL

gauntface · 2017-08-14T17:36:27Z

.travis.yml

+  - export PATH=$PATH:./node_modules/.bin
+  - mkdir -p tests/config && echo "$PROJECT_CONFIG" > tests/config/project.json
+script:
+  gulp test --suite="$TEST_SUITE" --env="$TEST_ENV"


TEST_SUITE doesn't seem to be defined anywhere

Another good catch, thanks!

gauntface · 2017-08-14T17:41:07Z

integration/serviceWorker/serviceWorker.js

@@ -14,7 +14,7 @@
 * limitations under the License.
 */

-importScripts('../../../dist/package/firebase-app.js');
+importScripts('./node_modules/firebase/firebase-app.js');


why use the node_modules instance of firebase here? Would have thought you'd need this to be dist

I npm install the firebase package that we build in dist here so we can consume it as a third party would for the test.

Are you running npm link first? While I follow the logic, I can see this magic biting someone later on.

gauntface · 2017-08-14T17:43:06Z

integration/typescript/runner.sh

@@ -0,0 +1,42 @@
+#!/bin/bash


could this just be a gulp task with multiple steps?

Yes and no. I opted to use bash instead as gulp pipelines discourage the use of "temp" directories which is what makes this flow so consistent. I could do that in gulp but it would be going against a best practice.

In addition, not everyone will be consuming the SDK via gulp and not all of the scripts will be possible in gulp (e.g. reactnative/cordova testing which is soon to come) so I lean towards bash for this.

gauntface · 2017-08-14T17:44:09Z

tests/storage/browser/request_test.ts

@@ -78,6 +77,7 @@ describe('Firebase Storage > Request', () => {
          const expectedHeaders = {};
          expectedHeaders[requestHeader] = requestValue;
          expectedHeaders[versionHeaderName] = versionHeaderValue;
+          console.log(args[4], expectedHeaders);


this and the follow log can be deleted.

Great catch, thank you!

…veraging their own binaries

refactor(quickstart-tests): refactor to use FIREBASE_PROJECT as an env var rather than an arg fix(tests): ensure that the wdio binary is pointing to the proper location fix(deps): add dep to quickstart package.json refactor(deps): move ts-node dep to devDeps fix(quickstart-tests): run tests on saucelabs WIP: refactor to remove reload (causing flakiness issues) refactor(quickstart): refactor to only navigate to the base URL instead of reloading the page fix(quickstarts): fix timing issue with reloads WIP: try limiting the scope to the :first-child WIP: refactor the tests to reference the intended post directly rather than indirectly. WIP: refactor to not iterate

sphippen

The Storage change looks good (I should actually trim some of that code, since it's leftover from internal use, irrelevant for the public SDK).

hiranya911

Took a high-level look at the shell scripts and config files.

hiranya911 · 2017-08-15T23:10:50Z

.travis.yml

+
+# Misc Addons/Configs
+dist: trusty
+sudo: required


Why is sudo required?

hiranya911 · 2017-08-15T23:11:31Z

.travis.yml

+  - npm install -g yarn
+before_script:
+  - export PATH=$PATH:./node_modules/.bin
+  - mkdir -p tests/config && echo "$PROJECT_CONFIG" > tests/config/project.json


How does PROJECT_CONFIG get initialized?

hiranya911 · 2017-08-15T23:20:14Z

integration/README.md

+
+## Authoring an Integration Test
+
+The point of the integration tests is to be as flexible as possible. The only convention that I would recommend is supplying a `runner.sh` file to do the actual work of the test.


we recommend

hiranya911 · 2017-08-15T23:20:28Z

integration/README.md

+- Service Worker Integration
+- Compatability with Webpack/Browserify/Typescript etc
+- High level UI testing
+- Others with same kinds of environmental restriction


restrictions

hiranya911 · 2017-08-15T23:21:23Z

integration/README.md

+That said, if you are looking for a pattern to follow, the process that was followed for several of the tests, is outlined below:
+
+- Copy the test files to a temporary working directory
+- Setup an `EXIT` trap to cleanup the temporary directory at the end of the script


Can we provide a reusable bash function in shared for this?

hiranya911 · 2017-08-15T23:22:35Z

integration/README.md

+- `npm install` the folder `dist/package`, this allows the test to function as if the SDK was installed from NPM
+- (For tests that are only validating the firebase namespace) copy the `shared` directory above into the working directory
+
+The tests are then ran with karma/mocha/webdriver.io/selenium or whatever else you need to accomplish the purpose of your test. Each of these runners requires their own config (and sometimes their own dependencies), supply these config files directly in the test directory (e.g. many of the suites have their own package.json) as the tests are meant to be isolated from each other as much as possible.


... own dependencies). Supply these config...

hiranya911 · 2017-08-15T23:23:57Z

integration/browserify/runner.sh

@@ -0,0 +1,45 @@
+#!/bin/bash


Might be a good idea to do set -e in these bash scripts so that any errors will trigger an early exit.

hiranya911 · 2017-08-15T23:24:37Z

integration/quickstart/src/test.json

@@ -0,0 +1,3 @@
+{
+  "text": "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Praesent tempor elit mi, nec ullamcorper neque lacinia vitae. Nulla porttitor ac mauris a mollis. Praesent pellentesque lacinia metus, in commodo risus vulputate eget. Vestibulum volutpat ullamcorper porta. Phasellus fringilla faucibus nisi, at sodales dolor maximus nec. Integer quis purus sed urna viverra pellentesque. Suspendisse potenti. Phasellus lobortis sagittis metus quis accumsan. Suspendisse pulvinar pretium mauris, quis congue risus scelerisque ac. Morbi quis placerat ante, in congue elit. Suspendisse et enim dictum, varius est quis, hendrerit nibh. Fusce vitae molestie sapien. Duis finibus cursus quam sed blandit."
+}


Add new line. Here and in other affected files.

jshcrowthe added 3 commits August 12, 2017 22:49

refactor(tests): refactor test harness to be more isolate

c069d8f

jshcrowthe requested a review from sphippen as a code owner August 13, 2017 05:55

docs(integration-tests): add README.md to explain high level concepts

7bc1e0b

jshcrowthe force-pushed the test-refactor branch from 778fd7d to 16d9761 Compare August 14, 2017 16:52

test(integration): add service worker testing

aaa3485

jshcrowthe force-pushed the test-refactor branch 2 times, most recently from a9a2284 to aaa3485 Compare August 14, 2017 17:02

jshcrowthe assigned gauntface Aug 14, 2017

gauntface reviewed Aug 14, 2017

View reviewed changes

firebase deleted a comment from jshcrowthe Aug 14, 2017

gauntface reviewed Aug 14, 2017

View reviewed changes

refactor(tests): better isolate the tests from the main package by le…

64f39ff

…veraging their own binaries

jshcrowthe force-pushed the test-refactor branch from fbcf52c to 64f39ff Compare August 14, 2017 18:26

jshcrowthe added 2 commits August 14, 2017 13:49

fix(storage): remove unnecessary console.log statements

c8365d7

fix(travis.yml): remove unneeded test script argument

1f5f180

jshcrowthe assigned hiranya911 and unassigned gauntface Aug 14, 2017

jshcrowthe force-pushed the test-refactor branch from 7fe90b7 to c0e76f9 Compare August 15, 2017 21:03

jshcrowthe merged commit bca804a into master Aug 15, 2017

jshcrowthe deleted the test-refactor branch August 15, 2017 21:21

sphippen reviewed Aug 15, 2017

View reviewed changes

hiranya911 self-requested a review August 15, 2017 23:04

hiranya911 reviewed Aug 15, 2017

View reviewed changes

jshcrowthe mentioned this pull request Aug 17, 2017

Add ability to test a single SDK piece #127

Closed

firebase locked and limited conversation to collaborators Oct 26, 2019


		## Authoring an Integration Test

		The point of the integration tests is to be as flexible as possible. The only convention that I would recommend is supplying a `runner.sh` file to do the actual work of the test.

Integration Test Refactor #131

Integration Test Refactor #131

Uh oh!

Conversation

jshcrowthe commented Aug 13, 2017

Uh oh!

jshcrowthe commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshcrowthe Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sphippen left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jshcrowthe Aug 14, 2017 •

edited

Loading