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

Tests for layer filter feature #424

Merged

Conversation

crisner
Copy link
Contributor

@crisner crisner commented Feb 28, 2020

Fixes #423 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@crisner
Copy link
Contributor Author

crisner commented Feb 28, 2020

I am running in to the following error for the tests I've commented out.
cyWindow
Looks like Cypress doesn't like us manipulating the hash and it breaks out of the iframe. But I can't seem to figure out why some tests pass while it triggers the error for the rest. This seems to be an ongoing issue. - cypress-io/cypress#1244

I tried to run the tests without the hash using the test.html file using cy.visit but I am having issues with the DOM style attributes not being loaded. I tried a wait time of upto 15 seconds and it still gives me the same error. The same tests pass in the previous method.
cyVisit

@crisner crisner force-pushed the testing/add-tests-for-layer-filtering branch from e7eb367 to 0a8a9b8 Compare February 28, 2020 07:37
@crisner
Copy link
Contributor Author

crisner commented Mar 3, 2020

Can't seem to get it to work, unfortunately. :( What do you think @sagarpreet-chadha? Do you think we should check the filtering for every single layer or just that the filtering works? If its the latter we've already done that in layerMenu.spec.js:

it('adds layers to the menu on map movement', function() {

it('removes layers from the menu on map movement', function() {

@jywarren
Copy link
Member

jywarren commented Mar 3, 2020

I think it's fine to test the general functionality of the filter and not intertwine it with each individual layer. I appreciate your linking to the relevant Cypress test! Hashes are a little bit undersupported in general as a UI, so it's not super surprising. So, this should be good to go then? Thanks!!

@sagarpreet-chadha
Copy link
Collaborator

Yes let's not go deep into each layer file filtering. Great efforts 🙌 !!!

@crisner
Copy link
Contributor Author

crisner commented Mar 4, 2020

Great! The general functionality for layer filtering has already been tested in the layersMenu spec. We could close this PR since it has already been done. The only difference is in the spec included in this PR I am not only checking the layer count but also the layers individually.

@crisner
Copy link
Contributor Author

crisner commented Mar 4, 2020

These tests would fail because of the recent changes if we merge with master. I'll push the fix in case we decide to keep it.

@crisner crisner force-pushed the testing/add-tests-for-layer-filtering branch 2 times, most recently from 916f5df to f3a9839 Compare March 4, 2020 13:22
@crisner crisner force-pushed the testing/add-tests-for-layer-filtering branch from f3a9839 to 2b0a083 Compare March 4, 2020 13:24
@crisner
Copy link
Contributor Author

crisner commented Mar 4, 2020

Sorry about that. These tests will have errors only when PR #428 merged. I guess I am a bit disoriented seeing how much I confused myself over this. 😅

@jywarren jywarren merged commit 76f4e38 into publiclab:master Mar 6, 2020
@jywarren
Copy link
Member

jywarren commented Mar 6, 2020

OK, awesome!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write tests to check layer filtering for all layers
3 participants