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

chore: add cypress #27

Closed
wants to merge 16 commits into from
Closed

chore: add cypress #27

wants to merge 16 commits into from

Conversation

JacobMGEvans
Copy link
Contributor

@JacobMGEvans JacobMGEvans commented May 26, 2020

5/24/20

  • initial setup, current test assert true is true
  • next step is creating mock data for fields and utilizing the inputs

5/25/20

  • Added data-cyp on the suggested query in component QueryAdvise
  • Added comments to seed file (current unused mostly)
  • Added two tests for smoke testing app main functionality

5/29/20
dda125b

  • Updates to ESlint merged
  • Updates to package.json merged
  • Change in lock incoming change added with current

72eac23

  • Fixed Linter Errors -- unused variables

960a767

  • Tests cover the functionality of query options
  • Clearing and user input of the code fields
  • Clicking on the HTML previews
  • Validating the query option selections change the descriptive content

JacobMGEvans and others added 6 commits May 24, 2020 10:53
- initial setup, current test assert true is true
- next step is creating mock data for fields and utilizing the inputs
- Added data-cyp on suggested query in component QueryAdvise
- Added comments to seed file (current unused mostly)
- Added two tests for smoke testing app main functionality
@smeijer
Copy link
Member

smeijer commented May 26, 2020

Nice! Can you let me know if/when this is ready for review? I'm not sure, because I've noticed that there are a bunch of fixtures committed, that we don't seem to use.

@JacobMGEvans
Copy link
Contributor Author

Yeah, that stuff is auto-generated by cypress, I tried removing it but it seems if to replace it if there isn't something there.
I plan to continue working on this but figured it was a good initial point, so I would say it's ready as a starting point.

@smeijer
Copy link
Member

smeijer commented May 26, 2020

Deleting is possible, but requires you to add an empty config file.

Just create an cypress.json in the project root, containing nothing more than {}. Then you can delete those files :)

edit And now I see you already have a config file, in another project of mine, that was the fix. 😕

@JacobMGEvans
Copy link
Contributor Author

Yeah, but I will end up using fixtures soon anyways, this was mostly an initial setup with a smoke test.

- Utilized fixture for constant HTML
- Refactored utilizing fixture
- Currently tests rewrite of a demo HTML and then clicks on HTML preview for working button
@JacobMGEvans
Copy link
Contributor Author

@smeijer Updates:

  • Added fixtures
  • Refactored
  • Abstracted reusable functions to support
  • Improvements to some of the testing (less brittle hopefully)

@smeijer
Copy link
Member

smeijer commented May 28, 2020

Thanks 👍 Just wanted to let you know that I've seen it. I need a bit more time for a good check though. I'll come back to this.

@smeijer smeijer changed the title Feature/cypress feat: add cypress May 28, 2020
@smeijer smeijer mentioned this pull request May 28, 2020
@JacobMGEvans
Copy link
Contributor Author

I will have to resolve recent conflicts or someone else can, I am good either way. I won't be able to get to it tonight most likely.

@smeijer smeijer changed the title feat: add cypress chore: add cypress May 29, 2020
- Updates to ESlint merged
- Updates to package.json merged
- Change in lock incoming change added with current
@JacobMGEvans
Copy link
Contributor Author

I will have to resolve recent conflicts or someone else can, I am good either way. I won't be able to get to it tonight most likely.

@smeijer I resolved the conflicts 😄

- Tests cover functionality of query options
- Clearing and user input of the code fields
- Clicking on the HTML previews
- Validating the query option selections change the descriptive content
@smeijer
Copy link
Member

smeijer commented May 30, 2020

Awesome! Thanks. I'll try to take a look at it this weekend.

cypress.json Outdated
@@ -0,0 +1,5 @@
{
"baseUrl": "http://localhost:1234",
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to run prettier again here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the husky hook running the linting and prettier application wide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked it is not running on Husky hooks and apparently the root files are being ignored by Husky.

@JacobMGEvans
Copy link
Contributor Author

I can definitely work on that next! 😄

@JacobMGEvans
Copy link
Contributor Author

JacobMGEvans commented May 30, 2020

I was definitely waist-deep in Cypress docs and totally forgot about Testing Libs Cypress Library! lmao duh! Thanks for the reminder and suggestion.

@JacobMGEvans
Copy link
Contributor Author

@smeijer Would you like that implemented in this PR before it goes in or as a next milestone for the Cypress integration?

@smeijer
Copy link
Member

smeijer commented May 30, 2020

I would prefer to have it in this PR, before we merge.

The codebase is moving quite fast, as the first thing I've shipped was more of a prototype level, than a production-grade thing.

Because of that, a lot of things are changing. ClassNames is one of those things. If we would merge this pull-request in it's current state, I'm sure tests will break all the time.

And then it would slow us down, instead of giving assurance that everything still works as it should be.

@smeijer smeijer changed the base branch from master to develop May 31, 2020 12:48
@smeijer smeijer marked this pull request as draft June 5, 2020 14:25
@JacobMGEvans
Copy link
Contributor Author

Sorry about the absence, I had to take a break from coding outside of work. I will get back to this soon.

@smeijer
Copy link
Member

smeijer commented Jun 10, 2020

Sure. I completely understand. 😊 Take your time.

@smeijer
Copy link
Member

smeijer commented Oct 9, 2020

Any wise words on what to do with this PR? I would love to have some e2e tests, but I have no Idea how much work it would be to bring this PR to a workable state.

@ddehart
Copy link
Contributor

ddehart commented Dec 2, 2020

@smeijer I figured I'd take a stab at this as a part of learning more about testing JavaScript, and the markup -- specifically the implementation of codemirror -- makes it challenging to make use of the principles of Testing Library.

Acknowledging that I'm a complete novice, attempting something even as simple as selecting the editor to clear it out and type something as a user would may be impossible without brittle selectors or acrobatics like grabbing parents of parents. I even went so far as trying to use Testing Playground on Testing Playground to identify good selectors without much luck. 😂

Someone with a bunch more Cypress/Testing Library experience can probably find an elegant solution, but I couldn't even really get started without considering class selectors or thinking about injecting a bunch of testids into the markup.

I hope that perspective helps as this moves forward!

@smeijer
Copy link
Member

smeijer commented Dec 2, 2020

Someone with a bunch more Cypress/Testing Library experience can probably find an elegant solution, but I couldn't even really get started without considering class selectors or thinking about injecting a bunch of testids into the markup.

If using testIds or classNames is the only thing that can make this work (easily), then we should do that.

Does that mean that this branch works when we rebase it? We currently don't have many tests, so I'm eager to get this one merged.

@ddehart
Copy link
Contributor

ddehart commented Dec 2, 2020

If using testIds or classNames is the only thing that can make this work (easily), then we should do that.

How would you feel about having actual ids associated with the editable components? That would make selection easy, potentially less brittle than classnames, and not so specific to a testing framework.

@JacobMGEvans
Copy link
Contributor Author

If using testIds or classNames is the only thing that can make this work (easily), then we should do that.

How would you feel about having actual ids associated with the editable components? That would make selection easy, potentially less brittle than classnames, and not so specific to a testing framework.

I think as long as it's utilizing the https://testing-library.com/docs/cypress-testing-library/intro/ and following the accessibility priority, then IDs necessity should be minimized. I never got around to that, unfortunately.

@ddehart
Copy link
Contributor

ddehart commented Dec 2, 2020

I think as long as it's utilizing the https://testing-library.com/docs/cypress-testing-library/intro/

Oh, sorry, I was referring specifically to the HTML id attribute as opposed to a custom testid attribute. I'm just curious if HTML ids would be preferred for this project over testids

@smeijer
Copy link
Member

smeijer commented Dec 3, 2020

testIds and id serve the same purpose in this regard. So in that case, I would say that a testId is preferable, as it at least makes its intension clear.

@ddehart
Copy link
Contributor

ddehart commented Dec 6, 2020

@JacobMGEvans do you plan on rebasing with your changes? If not, I can take a stab at it.

@marcosvega91
Copy link
Member

This PR is very old, so I think that is better to start a new PR and copy what @JacobMGEvans has done here in the new one ? What do you think guys?

@smeijer
Copy link
Member

smeijer commented Dec 6, 2020

That's what a rebase does 😊. I'm fine with any solution. I think it's still good to have some tests. So any progress in this area would be awesome.

@JacobMGEvans
Copy link
Contributor Author

@JacobMGEvans do you plan on rebasing with your changes? If not, I can take a stab at it.

You can use this branch if you like or create your own from scratch. I had a lot of the setup done which was most of the pain. I think I needed to add https://testing-library.com/docs/cypress-testing-library/intro/ (CTL)

The current tests were mostly smoke tests, just needed that CTL, however, if you DO do it from scratch keep this in mind because it was a big hurdle with the CodeMirror
https://stackoverflow.com/questions/62012319/how-can-i-clear-a-codemirror-editor-field-from-cypress

@smeijer smeijer marked this pull request as ready for review December 6, 2020 15:53
@smeijer smeijer marked this pull request as draft December 6, 2020 15:54
@JacobMGEvans
Copy link
Contributor Author

I'll try to update the PR with upstream main, RN

@smeijer
Copy link
Member

smeijer commented Dec 6, 2020

The conflicting files doesn't seem to be much. Does it make sense to resolve the conflict, merge, and improve in a new PR?

@JacobMGEvans
Copy link
Contributor Author

Well, that is odd... The branch and work have disappeared. I know I didn't delete it.

@JacobMGEvans
Copy link
Contributor Author

I will figure this out later.

@smeijer
Copy link
Member

smeijer commented Dec 6, 2020

@JacobMGEvans, can it be because origin has changed in the mean time? It moved from smeijer/testing-playground to testing-library/testing-playground.

@JacobMGEvans
Copy link
Contributor Author

@JacobMGEvans, can it be because origin has changed in the mean time? It moved from smeijer/testing-playground to testing-library/testing-playground.

Very possible 😅 ! The repo was originally forked from that old one. I don't know what the effects would normally be but explains the "unknown repository." I also rebased already and the work might have been develop local I can try to undo that or find the changes in a detached HEAD if not the PR at least has most of the files changes here

@smeijer
Copy link
Member

smeijer commented Dec 7, 2020

Superseded by #313 (comment). Again, I'm very sorry that I haven't merged this sooner.

@smeijer smeijer closed this Dec 7, 2020
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.

5 participants