Run tests for browser #228

jasonpaulos · 2020-10-05T18:42:21Z

This PR allows our mocha tests and SDK unit and integration tests to run in browsers.

Primary changes:

Setting the environment variable TEST_BROWSER to either chrome or firefox and running npm test or make docker will run tests in that browser instead of Node. If you are running on a desktop, that browser will open to a testing window, which can be used to debug any failing tests too.
- The tests are still driven from the Node command line, but we use selenium-webdriver to execute test code in the browser. Any failed tests will report back an error to Node, however these errors are often less useful that what gets reported in the browser.
Chrome and Firefox testing have been added as parallel Travis jobs, however the Firefox job is not required to pass since it has errors--I've opened a separate issue (Add testing for firefox #230) to debug those.
SDK unit and integration tests no longer run inside of a docker container. This is because they now may need to interact with a browser, and that's harder to do if they run inside of a container. @winder let me know if you think this would cause any problems.

Secondary changes:

make docker is now just a wrapper around npm run docker-test. This allowed me to define a predocker-test npm script that builds the package and an auxiliary testing bundle and puts them in the right place for browser tests.
The logicsig templates in src/logicTemplates are now exported to algosdk.LogicTemplates.
In order to resolve a circular dependency after exporting the above, I moved most of the functions from src/main.js into separate files that can be required individually.
The Transaction class is now exported to algosdk.Transaction. This enables it to be tested in the browser after our package has been bundled.

Browser bugs fixed:

In the browser binary msgpack fields would sometimes be a Uint8Array, but a Buffer in Node. Now everything should be a Uint8Array.
Superagent in the browser seems to parse response bodies when the content type is text/plain, but in Node it uses a default value of {}. This caused an issue with the /health endpoint, which returns null with content type text/plain. I've submitted Set content type header for health endpoint go-algorand#1621 to fix this, and in the meantime just ignored the body of the health response.

Closes #211.

EvanJRichard · 2020-10-14T22:14:16Z

I think this is a needed refactor, and mostly LGTM. Kudos especially on resolving the circular dependencies/breaking down main.js. (Does this break backwards compatibility?) Withholding green checkmark until Will can comment on the docker stuff.

jasonpaulos · 2020-10-15T13:14:50Z

The refactor is internal only so it should not break backwards compatibility unless users require specific files in our package (which we don't support anyway).

…er mocha support

winder

This looks like a huge improvement across the board, nice work. Regarding the docker changes, I wonder why it can't be kept? Adding a more seamless way to run tests locally would be nice since it's significantly easier to use a debugger that way. But for my usual use case, it's nice that I don't have to setup npm on my workstation to run tests when I make minor iterations to things like client endpoints.

I've done a lot of experiments related to this on the java SDK:
https://github.com/algorand/java-algorand-sdk/blob/develop/run_integration_tests.sh

winder · 2020-10-16T13:48:22Z

Makefile

@@ -1,7 +1,7 @@
 unit:
-	node_modules/.bin/cucumber-js --tags "@unit.offline or @unit.algod or @unit.indexer or @unit.rekey or @unit.tealsign or @unit.dryrun or @unit.applications or @unit.responses or @unit.transactions" tests/cucumber/features --require tests/cucumber/steps/*
+	node_modules/.bin/cucumber-js --tags "@unit.offline or @unit.algod or @unit.indexer or @unit.rekey or @unit.tealsign or @unit.dryrun or @unit.applications or @unit.responses or @unit.transactions" tests/cucumber/features --require tests/cucumber/steps/index.js


Was there a technical reason this needs to be fully specified? Ideally these tests would split these tests into multiple files someday.

Never mind, I just got to index.js. I guess you found that cucumber-js has a different mechanism for this.

winder · 2020-10-16T13:52:30Z

src/convert.js

@@ -0,0 +1,30 @@
+const MICROALGOS_TO_ALGOS_RATIO = 1e6;
+const ERROR_INVALID_MICROALGOS = new Error("Microalgos should be positive and less than 2^53 - 1.");


Is this still true after the bigint changes?

@EvanJRichard did you make a change here that got lost in the rebase, or is leveraging the BigInt support within the SDK still in progress?

I missed that this error existed, we could change the check in this PR or another one

winder · 2020-10-16T13:54:10Z

src/encoding/address.js

@@ -41,7 +41,12 @@ function isValidAddress(address) {
    return utils.arrayEqual(checksum, decoded.checksum);
 }

-function decode(address) {


Unfortunately it isn't a good idea to rename/remove public functions in an SDK. This could break other peoples code. You could add decodeAddress/encodeAddress and change encode/decode to print a deprecation warning before calling the new function.

These specific functions were used internally as address.decode/encode but exported by main.js in wrapper functions decodeAddress/encodeAddress. As part of the refactor I did, I thought it would be simpler to give them their public names from the beginning to avoid renaming later. So the end user shouldn't notice a difference here, unless they specifically import the internal file algosdk/src/encoding/address.

In JS it's best practice for modules to export all public functions & variables from their main entrypoint (main.js in our case), so I don't think we should support users importing our internal files. These internal files aren't even accessible in the browser from the bundle we build, so it would be unfair to expect users to have to do that anyway to access parts of the SDK.

winder · 2020-10-16T13:57:43Z

src/transaction.js

 }

-module.exports = {Transaction, TxGroup, ALGORAND_MIN_TX_FEE};


This could break other peoples code also, is it possible to forward TxGroup through this file?

Also IIRC some of these functions were intentionally kept private, Rotem should probably take a look at this to verify that we aren't exposing more than the design calls for.

Similar to my above reply, I believe changing where TxGroup is defined should not break any supported uses of the SDK since it only changes internal files.

Re: exporting functions, I will change it so that TxGroup is not publicly exported, since it is only used by our mocha tests, which can import internal files. However Transaction is used by cucumber steps, which can't import internal files because it runs against our bundled browser code. If it's important that this class does not get publicly exported, I can rewrite the two steps that use it.

winder · 2020-10-16T14:05:53Z

tests/cucumber/docker/sdk.py

@@ -1,25 +0,0 @@
-#!/usr/bin/env python3


Nice catch, this is from an old iteration of our testing framework 👍

jasonpaulos · 2020-10-16T15:37:37Z

@winder I can see the benefit of running the tests in a container now. I will look into how we can still support that for browser tests too. The main reason I chose to get rid of running the tests in a container is because it's easier to setup WebDriver when the process running the tests, browser, and WebDriver driver for that browser are all on the same machine and I wasn't aware of any benefits of running our tests in a container.

winder

This sounds good to me. I think it's useful to keep docker, but see what you can do.

jasonpaulos force-pushed the jasonpaulos/test-browser branch from 2f12f84 to 9a9dff5 Compare October 14, 2020 15:56

jasonpaulos marked this pull request as ready for review October 14, 2020 19:01

jasonpaulos requested review from EvanJRichard and winder October 14, 2020 19:09

jasonpaulos force-pushed the jasonpaulos/test-browser branch from 9c02945 to 8381e91 Compare October 15, 2020 15:28

jasonpaulos added 17 commits October 15, 2020 15:11

Use npm ci

7a76aba

Remove unnecesary test imports

6fddc8c

Break up step definition file

20566ce

Unify resource loading

2792ac2

Add cors headers to mock server

c994059

Support for cucumber unit tests in browser

c93de7c

Add browser tests to travis

4d659f2

Make binary responses consistent

c6f5ea6

Run integration tests

0f7b896

Export logic templates

9449dfc

Add firefox support, refactor to avoid circular references, add brows…

b14be13

…er mocha support

Update gitignore

1c15950

Fix failures in chrome integration tests

adcfbba

Update test scripts and README

ca90dcf

Add comment to step definition file

46bffda

Clean up docker files

7fb6ef5

Fix test environment log

415e19c

jasonpaulos force-pushed the jasonpaulos/test-browser branch from 840cf9a to 415e19c Compare October 15, 2020 19:13

winder reviewed Oct 16, 2020

View reviewed changes

winder approved these changes Oct 16, 2020

View reviewed changes

rotemh approved these changes Oct 17, 2020

View reviewed changes

Remove TxGroup from exports

8640f40

jasonpaulos added 5 commits October 19, 2020 13:39

Put test runner back in docker

4e5225f

Update travis script

cecf8b8

Fix docker network

2d96faf

Try using host network

714a346

Install browsers inside of test container

a5c4e9f

jasonpaulos merged commit 1d42168 into develop Oct 20, 2020

jasonpaulos deleted the jasonpaulos/test-browser branch October 20, 2020 13:50

jasonpaulos mentioned this pull request Oct 22, 2020

Remove unnecessary code from docker script #234

Merged

jasonpaulos mentioned this pull request Nov 3, 2020

v1.8.0 #238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests for browser #228

Run tests for browser #228

jasonpaulos commented Oct 5, 2020 •

edited

Loading

EvanJRichard commented Oct 14, 2020

jasonpaulos commented Oct 15, 2020

winder left a comment •

edited

Loading

winder Oct 16, 2020

winder Oct 16, 2020

winder Oct 16, 2020

jasonpaulos Oct 16, 2020

EvanJRichard Oct 16, 2020

winder Oct 16, 2020

jasonpaulos Oct 16, 2020

winder Oct 16, 2020

jasonpaulos Oct 16, 2020

winder Oct 16, 2020

jasonpaulos commented Oct 16, 2020

winder left a comment

		@@ -0,0 +1,30 @@
		const MICROALGOS_TO_ALGOS_RATIO = 1e6;
		const ERROR_INVALID_MICROALGOS = new Error("Microalgos should be positive and less than 2^53 - 1.");

		}

		module.exports = {Transaction, TxGroup, ALGORAND_MIN_TX_FEE};

Run tests for browser #228

Run tests for browser #228

Conversation

jasonpaulos commented Oct 5, 2020 • edited Loading

EvanJRichard commented Oct 14, 2020

jasonpaulos commented Oct 15, 2020

winder left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonpaulos commented Oct 16, 2020

winder left a comment

Choose a reason for hiding this comment

jasonpaulos commented Oct 5, 2020 •

edited

Loading

winder left a comment •

edited

Loading