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

E2e test mika #2931

Merged
merged 98 commits into from Jan 23, 2024
Merged

E2e test mika #2931

merged 98 commits into from Jan 23, 2024

Conversation

mika-robots
Copy link
Collaborator

Description
This is another attempt at adding cypress e2e test.
Few notes:

  • The coverage was removed. It didn't not seem to produce accurate results and we should spend some time on doing properly before re-enabling
  • There are few explicit waits which I could't avoid. They are related to clicking on the map to create locations

Jira link:

Type of change

Non breaking change that enable automated cypress tests
There are no changes to any files outside of end-to-end folder
How Has This Been Tested?

I run it both locally and on my branch

Checklist:

[ x] I have commented my code, particularly in hard-to-understand areas
[ x] My changes generate no new warnings
[ x] I have added tests that prove my fix is effective or that my feature works
[ x] New and existing unit tests pass locally with my changes
[ x] The precommit and linting ran successfully
[ x] I have added or updated language tags for text that's part of the UI
[ x] I have added "MISSING" for all new language tags to languages I don't speak
[ x] I have added the GNU General Public License to all new files

@mika-robots mika-robots requested review from a team as code owners October 26, 2023 22:30
@mika-robots mika-robots requested review from antsgar and kathyavini and removed request for a team October 26, 2023 22:30
Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

I'm sorry for taking so long to review this!!

I think this looks awesome and the more modular test design is 👌 . Since we took so long to get to this, some selectors and behaviours are now out of date -- I noted a few below -- but I would love to merge first to get this into the repo, and then open an update PR or two to implement those needed changes, which will touch on packages/webapp as well.

// Load the users fixture before the tests
cy.fixture('e2e-test-users.json').then((loadedUsers) => {
users = loadedUsers;
const user = users[Cypress.env('USER')];
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the correct way to handle the Cypress environment variables for running the tests locally?

I had to put USER into cypress.config.js but I don't think that was the intended method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, these actually should be added either in the configuration file, or in its own separate env variables file. There's an overview here on the different methods and their pros/cons https://docs.cypress.io/guides/guides/environment-variables#Setting

const uniqueSeed = Date.now().toString();
const uniqueId = Cypress._.uniqueId(uniqueSeed);

cy.get('[data-cy=home-farmButton]')
Copy link
Collaborator

Choose a reason for hiding this comment

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

home-farmButton is one of the many test ids that were lost in the navigation revamp (well actually in this case the whole menu was removed) so these tests will need a new way to select the people menu option.

cy.contains(translation['INVITE_USER']['CHOOSE_ROLE'])
.parent()
.find('input')
.type(roles['MANAGER'] + '{enter}');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure that typing into the React Selects like this was not intended, but was probably possible due to a bug in our code that existed until this commit (see also PR #2951 for more context).

Now this fails and will need a new method.


it('CheckTasksNavigation', () => {
// Confirm that location exists
cy.get('[data-cy=home-farmButton]')
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above; needs new selectors for the menu items.


// cy.get('[id$=react-select-3-input]').type(language+'{enter}');
cy.contains('English')
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above; this will fail because the React Select will not permit typing (is not searchable) anymore.

Cypress.Commands.add('acceptSlideMenuSpotlights', (crop_menu_name) => {
// Check the spotlights
cy.get('[data-cy=spotlight-next]').should('exist').and('not.be.disabled').click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we have one less spotlight here now.


Cypress.Commands.add('createFirstLocation', (fieldString) => {
cy.get('[data-cy=home-farmButton]').should('exist').and('not.be.disabled').click({ force: true });
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could store these selector strings somewhere to avoid having to update them in several different files when there's a UI change, e.g.

const FARM_BUTTON_SELECTOR = '[data-cy=home-farmButton]'

# - integration
pull_request:
branches:
- e2e_test_mika
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will dig into this a bit more, but it seems the answer to setting this up so that it only runs before merging to integration would be merge queues https://medium.com/@kojoru/how-to-set-up-merge-queues-in-github-actions-59381e5f435a

});
});

after(() => {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could probably be removed?

// Select field
cy.contains('First Field').should('be.visible');
// eslint-disable-next-line cypress/no-unnecessary-waiting
cy.wait(500, { log: false });
Copy link
Collaborator

Choose a reason for hiding this comment

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

If at all possible we should try not to have explicit waiting


after(() => {});

it('AddCropVariety', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT but I'd prefer if we made these test names a little bit more semantic describing what it is that the user would be doing that we're trying to check for, something like it('can add a new crop variety')

Comment on lines +8 to +28
cy.fixture('e2e-test-users.json').then((loadedUsers) => {
users = loadedUsers;
const user = users[Cypress.env('USER')];

// Load the locale fixture by reusing translations file
cy.fixture('../../../webapp/public/locales/' + user.locale + '/translation.json').then(
(data) => {
// Use the loaded data
translation = data;

cy.visit('/');
cy.loginOrCreateAccount(
user.email,
user.password,
user.name,
user.language,
translation['SLIDE_MENU']['CROPS'],
translation['FARM_MAP']['MAP_FILTER']['GARDEN'],
);
},
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This chunk of code might end up repeated throughout different files (it's currently both in crops and farm people), so it'd be good to split it into its own utility function or custom Cypress command https://docs.cypress.io/api/cypress-api/custom-commands

});

Cypress.Commands.add('acceptSlideMenuSpotlights', (crop_menu_name) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These last three commands, addFarm, onboardCompleteQuestions and acceptSlideMenuSpotlights are not reused in other test files but just by other commands here. With that in mind, they could probably just be regular functions instead of commands https://docs.cypress.io/api/cypress-api/custom-commands#1-Dont-make-everything-a-custom-command

@antsgar antsgar merged commit d4ae499 into integration Jan 23, 2024
3 checks passed
@antsgar antsgar deleted the e2e_test_mika branch January 23, 2024 20:17
kathyavini added a commit that referenced this pull request Jan 30, 2024
kathyavini added a commit that referenced this pull request Jan 30, 2024
kathyavini added a commit that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants