fix(network-shim): ensure in-test fixtures are used instead of shim fixtures #176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Mohammer5 thanks for the additional commit. Would you like to request changes? If not, please approve so we can release this fix. |
|
I tested it with the sms config app and it didn't work "plug'n'play" style. Capturing seems to have worked, but during stub mode, the tests did not login successfullly:
I had to make some adjustments to the sms config app to work with this version of the cli utils as they use cypress@7 and the sms config app still used v5 of our tool, the branch is: Prepare_network_shim |
HendrikThePendric
force-pushed
the
CLI-31-ensure-in-test-intercept-fixtures-are-returned-instead-of-network-shim-fixtures
branch
from
e6ef07b to
fdcac39
Compare
HendrikThePendric
force-pushed
the
CLI-31-ensure-in-test-intercept-fixtures-are-returned-instead-of-network-shim-fixtures
branch
2 times, most recently
from
300d802 to
ded9fc2
Compare
…ixtures The core issue was fixed by upgrading cypress to v7, which includes the correct overriding behaviour for `cy.intercept`. Due to the major version bump, capturing and stubbing requests stopped working, and this was fixed by passing regexes to cy.intercept instead of strings. A new test was introduced to the network-shim example-app to assert the in-test fixtures take presidence over the networkshim fixtures. BREAKING CHANGE: bumps cypress 1 major version, to v7
This was planned anyway to keep fixtures stable. But also turned out to be required due to a bug: cypress-io/cypress#16420
HendrikThePendric
force-pushed
the
CLI-31-ensure-in-test-intercept-fixtures-are-returned-instead-of-network-shim-fixtures
branch
from
6cd2d67 to
841d2eb
Compare
Mohammer5
suggested changes
May 18, 2021
There was a problem hiding this comment.
The only real change requests I have are what we already discussed:
Please add docs about the quirk with testing the testing-network-shim-app example app:
npx --no-install cypress cache clear
find ./ -iname node_modules -type d | xargs rm -r
yarn install
Add docs or a script to test the whole network shim in both example apps
examples/platform-app/package.json
Outdated
| "cy:run": "d2-utils-cypress run --appStart 'yarn cy:start'", | ||
| "cy:capture": "d2-utils-cypress run --capture --appStart 'yarn cy:start' --serverMinorVersion 37", | ||
| "cy:stub": "d2-utils-cypress run --stub --appStart 'yarn cy:start' --serverMinorVersion 37", | ||
| "cy:stub-debug": "DEBUG=cypress:network:*,cypress:net-stubbing* d2-utils-cypress run --stub --appStart 'yarn cy:start' --serverMinorVersion 37" |
There was a problem hiding this comment.
Not sure if this should make it into the release, but it's not crucial functionality and not blocking any other script, so I don't have a strong opinion on this
Mohammer5
approved these changes
May 18, 2021
HendrikThePendric
deleted the
CLI-31-ensure-in-test-intercept-fixtures-are-returned-instead-of-network-shim-fixtures
branch
dhis2-bot
added a commit
that referenced
this pull request
May 18, 2021
# [8.0.0-alpha.1](v7.0.1...v8.0.0-alpha.1) (2021-05-18) ### Bug Fixes * **network-shim:** ensure in-test fixtures are used instead of shim fixtures ([#176](#176)) ([84a1907](84a1907)) ### BREAKING CHANGES * **network-shim:** bumps cypress 1 major version, to v7 * fix(network-shim): filter request and response headers properties This was planned anyway to keep fixtures stable. But also turned out to be required due to a bug: cypress-io/cypress#16420 * fix(network-shim): disable auto-login during stub run * fix(network-shim): add 'system/info' resource to static resources list * feat(network-shim): run tests suite on CI * docs(network-shim): add info about the network-shim test suite * chore(network-shim): add command to locally run full e2e suite * docs(network-shim): add info reg troubleshooting and local full test run * chore(cy local): run build command before cypress commands Co-authored-by: Jan-Gerke Salomon <[email protected]>
|
🎉 This PR is included in version 8.0.0-alpha.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
dhis2-bot
added a commit
that referenced
this pull request
Jun 10, 2021
# [8.0.0](v7.0.1...v8.0.0) (2021-06-10) ### Bug Fixes * **network-shim:** ensure DHIS2_BASE_URL is available in localStorage ([#214](#214)) ([741ab4b](741ab4b)) * **network-shim:** ensure in-test fixtures are used instead of shim fixtures ([#176](#176)) ([84a1907](84a1907)) * **network-shim:** fix before hook bug ([#201](#201)) ([0e1cd4c](0e1cd4c)) * **network-shim:** only incrementally update missing request stub state ([#209](#209)) ([e2ccea8](e2ccea8)) * **network-shim:** report missing stubs if at least one is found ([#208](#208)) ([45b3331](45b3331)) * **network-shim:** use electron instead of chrome for runs ([#213](#213)) ([ae73686](ae73686)) ### chore * remove node 10 support ([6245ef2](6245ef2)) ### Code Refactoring * **install command:** combine network shim command & plugin options ([4bc9a4e](4bc9a4e)) * drop the app-start flag ([9674d87](9674d87)) * simplify cypress-plugin and cli-utils-cypress ([dc58462](dc58462)) * wait for baseUrl to become available ([745194f](745194f)) ### Features * **enable auto login:** add option to install command ([e9dde4e](e9dde4e)) * **install cmd:** warn about potentially missing peer depds (temporarily) ([c3046aa](c3046aa)) ### BREAKING CHANGES * **install command:** The two options are merged into one, which is now called "enableNetworkShim". * New minimum version for NodeJS is 12.x. * Drop run and open commands We want to be consistent with how Cypress runs locally and in CI and since we cannot use d2-utils-cypress in CI, we shouldn't run it through d2-utils-cypress locally either. * Change configuration keys to camelCase. - dhis2_username => dhis2Username - dhis2_password => dhis2Password - dhis2_base_url => dhis2BaseUrl - dhis2_datatest_prefix => dhis2DataTestPrefix - dhis2_api_version => dhis2ApiVersion * dhis2_api_stub_mode renamed to networkMode Instead of describing the mode of the plugin, it is a bit easier to understand if we speak in terms of the network: - do we want to capture the network traffic (networkMode=capture), - do we want to stub it (networkMode=stub), - or do we want to run it against a live backend (networkMode=live)? * 'DISABLED' renamed to 'LIVE' To better describe the state of the network when running tests instead of describing the state of the plugin, DISABLED is now LIVE. * isDisabledMode renamed to isLiveMode. Similar to the above, to better describe the state of the network vs. the state of the plugin, replace usages of isDisabledMode with isLiveMode. * 'CAPTURE'|'STUB'|'LIVE' are now lowercase when passed to the environment. Replace networkMode=LIVE with networkMode=live. * Drop the --waitOn flag As of now we wait on the baseUrl that is defined in cypress.json, as that is the URL that the tests are going to run against. * Drop support for the --appStart flag. As a consumer, you are expected to either use something like concurrently to run the app server and the cypress server in a single process, or run then manually in two separate processes. This is no longer done automatically. * **network-shim:** bumps cypress 1 major version, to v7 * fix(network-shim): filter request and response headers properties This was planned anyway to keep fixtures stable. But also turned out to be required due to a bug: cypress-io/cypress#16420 * fix(network-shim): disable auto-login during stub run * fix(network-shim): add 'system/info' resource to static resources list * feat(network-shim): run tests suite on CI * docs(network-shim): add info about the network-shim test suite * chore(network-shim): add command to locally run full e2e suite * docs(network-shim): add info reg troubleshooting and local full test run * chore(cy local): run build command before cypress commands Co-authored-by: Jan-Gerke Salomon <[email protected]>
|
🎉 This PR is included in version 8.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The initial issue was fixed by upgrading cypress to v7, which includes the correct overriding behaviour for
cy.intercept.Due to the major version bump there was a regression in both capture and stub mode: no requests were being intercepted by the network-shim at all. This was fixed by passing regexes to
cy.interceptinstead of strings.A new test was introduced to the network-shim example-app to assert the in-test fixtures take precedence over the network-shim fixtures.
Once capturing was working properly again, we encountered another regression due the major version bump: stub mode was producing network errors. After investigation it turned out the network errors were happening because of a
content-encodingproperty in the header. This is a cypress bug which I filed here: cypress-io/cypress#16420. This bug actually isn't that relevant for the network-shim because want to filter response and request headers properties anyway. I have implemented an allow-list mechanism and omittedcontent-encoding.Interestingly, we didn't encounter this cypress
content-encodingheaders until we tried the network-shim in the sms-configuration-app. Thetesting-network-shim-appexample app was unaffected. This was a reason to introduce a new DHIS2 platform example app. This new app also includes a cypress test, which functions as a smoke test for the functioning of the network-shim in a DHIS2 platform app.And finally, since it has become painfully clear that cypress changes a lot and that these changes might cause the network-shim to break, @varl and I have tweaked the GitHub workflows so that an e2e suite runs on CI. This should guard against regression. More details about the test have been added to the docs.
BREAKING CHANGE: bumps cypress 1 major version, to v7