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

Feature/cloud api testing #210

Merged
merged 57 commits into from Mar 12, 2019

Conversation

Projects
None yet
5 participants
@zone117x
Copy link
Member

commented Feb 26, 2019

Goal of PR

Perform integration testing for all drivers #197. This PR gives us the following:

  • Assurance that the revocation methodology of depending on cloud API 404s works reliably.
  • Implement more tests for ensuring driver/cloud API compliance and behavioral consistency.
  • Easier to implement/update/test drivers: a pattern for specifying mock driver code and integration config setup code now exists. These are automatically ran against our test suite ensuring driver compliance.
  • A json object for all cloud API credentials are stored in a private CircleCI env var.

Implementation

Normalized the method for developers to add to the set of mock driver implementations and the set of driver integration setup methods.

Each driver implementation has an entry for both A) mock instance setup method, and B) an method that instantiates an production driver instance when the credentials/config are specified via env vars.

Both these sets are iterated over, with each driver being instantiated then ran through the main driver unit test. In additional, several tests dealing with authentication and revocations are now also ran against all configured drivers.

Manual Testing

  1. Git pull
  2. Specify cloud API credentials using either the DRIVER_CONFIG_TEST_DATA env var pointed to the Driver Credentials JSON found in the Gaia Driver Testing Keys 1password vault (ask @kantai or myself for access). The circleCI tests are with this.
    • Alternatively, the previous (driver specific) env vars can still be used. For the the list of these env vars see:
    • If no cloud credentials are specified, the integration tests will still be ran using the self-hosted diskDriver and the InMemoryDriver.
  3. npm i
  4. npm test

Automated Testing

The disk driver now has a mock and self-hosted integration implemented. This, along with the InMemoryDriver, are always ran (no environment/external configuration needed). Addressing @jcnelson's earlier revocation PR comment about using the diskDriver rather than the InMemoryDriver - we can probably remove the InMemoryDriver now that we have this setup. However, this did create the first inter-project npm dev-dependency where the hub project now depends the the reader project. We should evaluate and confirm that we are OK with this.

zone117x added some commits Feb 22, 2019

Test coverage improvements:
* Run the same drive test suite on all drivers (both on the set of drivers with mock implementations, and the set of drivers that have real credentials specified).
* Implemented mock implementation test for the DiskDriver.
* Fixed DiskDriver write file returning immediately without waiting for file write to end/succeed/fail.
* Fix several sync bugs in DiskDriver that cripple Node's event loop under load.
Add InMemoryDriver to built-in integration tests, removed redundant t…
…ests for DiskDriver and InMemoryDriver.
Console log warning when DiskDriver is being used with a `config.buck…
…et` so user knows it is not being used.
Fix remaining bugs with synchronous fs API calls in DiskDriver. Make …
…main driver test exercise recursive directory reads.

@zone117x zone117x requested review from jcnelson and kantai Feb 26, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 1, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (develop@3b95fb8). Click here to learn what that means.
The diff coverage is 90.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #210   +/-   ##
==========================================
  Coverage           ?   90.43%           
==========================================
  Files              ?       12           
  Lines              ?      753           
  Branches           ?        0           
==========================================
  Hits               ?      681           
  Misses             ?       72           
  Partials           ?        0
Impacted Files Coverage Δ
hub/src/server/http.js 78.66% <100%> (ø)
hub/src/server/ProofChecker.js 100% <100%> (ø)
hub/src/server/utils.js 86.36% <82.35%> (ø)
hub/src/server/drivers/S3Driver.js 90.9% <84.21%> (ø)
hub/src/server/drivers/GcDriver.js 93.05% <89.18%> (ø)
hub/src/server/drivers/AzDriver.js 95.16% <93.47%> (ø)
hub/src/server/drivers/diskDriver.js 91.95% <94.87%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b95fb8...9fa4d99. Read the comment docs.

@zone117x

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

Several bugs were identified and fixed while writing these tests. Some of the fixes involved changes that I would have preferred to have submitted separate PRs for, however, many of them were tightly coupled with the changes required for the integration tests. The code coverage reports and significant increase in tests should hopefully make this PR review a bit easier.

Bug Fixes:

  • Fix pagination/pageSize bugs with AzDriver, S3Driver, and diskDriver.
  • Fixed bug in Azure and GoogleCloud drivers where performWrite did not propagate http upload stream errors - leaving hanging promises, event listeners, and/or open streams.
  • The disk driver contained a bug performWrite where the asynchronous pipe between the http POST stream and file-write-stream was created, abandoned, and a 2xx success http response being immediately sent. Any errors occurring during http upload or file writing would ignored. Also, this caused integration test failures when writing then reading since the file stream would not yet be flushed to disk.
  • The disk driver also contained several synchronous fs usages which could freeze up the main event loop under certain types of disk load. A mounted network drive comes to mind.
  • The reader server wasn't checking for path traversals (..) in the filename request.
  • Fixed a few edge-case errors occurring next to manual Promise chaining which would not get propagated through the async call stack.
Show resolved Hide resolved reader/.flowconfig

wileyj and others added some commits Mar 5, 2019

updating dockerfiles
- use lts-alpine
- add glibc for the admin image since flow requires it
@kantai

kantai approved these changes Mar 12, 2019

Copy link
Member

left a comment

This is awesome work, @zone117x

This all looks good to me, and the changes all worked when I tested locally.

@jcnelson

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Looks great to me! 🎉 🎉 🎉

@zone117x zone117x merged commit 0ab1ca5 into develop Mar 12, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
@moxiegirl

This comment has been minimized.

Copy link

commented Mar 12, 2019

@zone117x Do we want developers contributing to Blockstack to run these tests locally prior to submitting a PR --- and will they run automatically on a PR submission? Is there a use case outside of Gaia contributors for running them?

@zone117x

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

No. Requiring contributors to procure accounts and credentials for all cloud services is not reasonable. The full integration tests will be ran automatically once they submit a PR.

The default local tests (npm test) include integration tests using the disk driver and in-memory driver which should be good enough before submitting a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.