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

[cypress] Breakup and remove commands.js #7257

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

NickLaMuro
Copy link
Member

Breaks the file into multiple files of two different categories:

  • commands
  • assertions

This is mostly a pedantic commit, but I figured it made sense to do this now and set precedence before others start contributing and use this as a reference. If for whatever reason it is highly disliked (I guess if there is a contigency of peeps who really like "long line files" I am unware of), we can close as well.

Links

@NickLaMuro
Copy link
Member Author

@miq-bot assign @himdel

@NickLaMuro
Copy link
Member Author

@miq-bot add_labels cypress, refactoring

@NickLaMuro
Copy link
Member Author

@miq-bot add_label test

@miq-bot miq-bot added the test label Aug 11, 2020
@himdel
Copy link
Contributor

himdel commented Aug 11, 2020

I agree in principle, but depending on how pedantic you want to get.... :)

  • expect_explorer_title vs expect_show_list_title should ideally be just expect_title which can figure out where we are (so I'd keep them together)

  • menuItems is perhaps more of an assert than a command (but not sure how useful it is really, now that it reads window.ManageIQ.menu instead of parsing the info from the menu html)

  • accordion is perhaps more related to explorers than to menus

But the structure makes sense, so, feel free to -Wno-pedantic me :D 👍

EDIT: debride spec failed on network error, restarted

@NickLaMuro
Copy link
Member Author

@himdel I actually agree with all of the critique, so I will look at making the changes.

I was considering combining the titles myself, just wasn't sure what a good name was, but I will go with the "obviously simple" one you suggested.

@NickLaMuro
Copy link
Member Author

NickLaMuro commented Aug 11, 2020

Okay, upon further inspection, I am going to retract my changes to menuItems (was going to change it to something like expect_menu_items), but since it doesn't seem to be used in an assert-type-fashion currently:

cy.menuItems()
.then((menu) => {
cy.log(menu);
expect(menu.length > 9).to.equal(true);
expect(menu[0].title).to.equal('Overview');
expect(menu[0].items[1].title).to.equal('Reports');
expect(menu[2].items[1].items[3].title).to.equal('Virtual Machines');
});

Used to support some assertions, but doesn't actually do the asserting itself.

Maybe we can make helpers for that in the future, but I think for now, just getting the structure right.


That said, will still push the other two changes... hold plz.

Breaks the file into multiple files of two different categories:

- commands
- assertions
@NickLaMuro NickLaMuro force-pushed the refactor-cypress-commands branch from ecd206f to 271be06 Compare August 11, 2020 22:12
@miq-bot
Copy link
Member

miq-bot commented Aug 11, 2020

Checked commit NickLaMuro@271be06 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🏆

@himdel himdel merged commit 80724fc into ManageIQ:master Aug 12, 2020
Fryguy added a commit to Fryguy/manageiq-ui-classic that referenced this pull request Apr 21, 2022
This bug was introduced in ManageIQ#7257.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants