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

test(protocol-designer): add migration + load-save e2e tests #5369

Merged
merged 8 commits into from
Apr 8, 2020

Conversation

IanLondon
Copy link
Contributor

@IanLondon IanLondon commented Apr 3, 2020

overview

In our "PD Testing Requirements for Regular Release Cadence" doc, some points we outlined were

  • Atomic commands should not change surprisingly on load-and-save
  • PD must import saved files (current and past) corectly

This PR closes #5123 by recreating the v1 migration tests (disabled since v2.x.x release because labware fixtures interfered with protocol load-save process), as Cypress tests.

It also closes #5173, by ensuring that loading and saving a v3 or v4 protocol derives the same atomic commands from the data PD actually reads on import (applicationData). This prevents regressions where PD could unexpectedly generate different commands given the same step forms.

This should address the PD side of #5103, but the issue still exists in Labware Creator.

I also moved protocol fixtures to protocol-designer/fixtures/protocol/{schemaMajorVersionNum}/{fixtureName}.json because they're not only used in the load-file tests where they lived before. I thought about putting them in shared-data, but they are actually PD-specific because the shared-data fixtures don't have applicationData for PD, but the fixtures in this PR rely on applicationData.

The new "result" fixtures came from loading the old fixtures into PD and migrating them. They're effectively a snapshot.

changelog

  • add cypress-file-upload
  • move PD's fixtures around to different folder
  • add tests for v1 -> v3, v3 -> v3, v4 -> v4

review requests

Cypress doesn't have much support for checking if files are downloaded or loading up the downloaded file. cypress-io/cypress#949 So this PR relies on intercepting file save if Cypress is available, and putting the file data into a global variable that Cypress has access to.

Also, Cypress doesn't always show a deep diff, it very hard to figure out what went wrong when mismatched protocols cause a deepEqual. cypress-io/cypress#4084 So a kind-of-hack around this is using the expectDeepEqual utility.

  • Code review
  • PD should still save files normally in dev/production when Cypress is not available

risk assessment

Low, just PD tests

@IanLondon IanLondon added the protocol designer Affects the `protocol-designer` project label Apr 3, 2020
const savedFile = cloneDeep(window.__lastSavedFile__)
const expected = cloneDeep(expectedExportProtocol)

// TODO(IL. 2020-04-03): this will change to 4.0.x soon
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will have to be part of #5148

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #5369 into edge will increase coverage by 0.24%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5369      +/-   ##
==========================================
+ Coverage   63.70%   63.95%   +0.24%     
==========================================
  Files        1087     1087              
  Lines       30865    31164     +299     
==========================================
+ Hits        19662    19930     +268     
- Misses      11203    11234      +31     
Impacted Files Coverage Δ
protocol-designer/src/load-file/migration/1_1_0.js 81.69% <0.00%> (-2.37%) ⬇️
...designer/src/components/FileSidebar/FileSidebar.js 77.77% <66.66%> (-1.30%) ⬇️
...rotocol-designer/src/steplist/fieldLevel/errors.js 46.42% <0.00%> (-0.24%) ⬇️
app/src/robot/constants.js 100.00% <0.00%> (ø)
protocol-designer/src/steplist/fieldLevel/index.js 50.00% <0.00%> (ø)
...ponents/LiquidPlacementForm/LiquidPlacementForm.js 0.00% <0.00%> (ø)
app/src/robot/api-client/client.js 90.80% <0.00%> (+0.01%) ⬆️
app/src/robot/reducer/connection.js 93.10% <0.00%> (+2.19%) ⬆️
app/src/robot/selectors.js 84.21% <0.00%> (+3.46%) ⬆️
...col-designer/src/steplist/fieldLevel/processing.js 84.21% <0.00%> (+12.78%) ⬆️
... and 4 more

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 e3ce552...d386a26. Read the comment docs.

@IanLondon IanLondon removed blocked Ticket or PR is blocked by other work ready for review labels Apr 7, 2020
saveAs(blob, downloadData.fileName)
if (global.Cypress) {
// HACK(IL, 2020-04-02): can't figure out a better way to do this yet
// https://docs.cypress.io/faq/questions/using-cypress-faq.html#Can-my-tests-interact-with-Redux-Vuex-data-store
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dozercodes any ideas here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Ian and I discussed this a bit and I agreed that this is the best way to handle downloads right now. Cypress itself can't handle file downloads through the browser well (Selenium has the same problem). Another way we could get around this is by building a download link when we create the export that we can call cy.request() on, but that's simply another hack. This method is simpler, since we don't already generate download links, and achieves what we need (we don't need to test the browser download flow, just the contents of the file).

Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for doing this @IanLondon

As discussed, I am hesitant about checking the truthiness of global.Cypress. If for some reason that variable is truthy in a user's browser then it'll mess with file download, which is obviously very problematic.

I'll defer to @dozercodes here though, maybe lets just remember to come back to this to see if we can come up with an alternative way to check if Cypress is available.

}

// deepEqual won't always return a diff, Cypress doesn't fully support object diffs :(
// also Cypress doesn't seem to support logging to the console? So throwing the diff as an error instead
Copy link
Contributor

@dozercodes dozercodes Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, really? I was able to log to console in one of my tests, so I'm not sure why it wouldn't work here
Edit: I misunderstood. This makes sense now.

Copy link
Contributor

@dozercodes dozercodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good stuff here

Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📞

@IanLondon IanLondon merged commit b65b6a2 into edge Apr 8, 2020
@IanLondon IanLondon deleted the pd_add-migration-e2e-tests branch April 8, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol designer Affects the `protocol-designer` project
Projects
None yet
4 participants