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 test to create CWU opportunity #111

Closed
wants to merge 64 commits into from
Closed

Cypress test to create CWU opportunity #111

wants to merge 64 commits into from

Conversation

BCerki
Copy link
Collaborator

@BCerki BCerki commented Dec 21, 2021

This PR partially addresses issue: #107

Includes tests? y
Rebased on main address to conflicts? n/a
Updated docs? y

Proposed changes:

  • write a Cypress test that checks if a gov user can create and save a draft of a CWU opportunity. This involved the following pre-reqs:
    -- add an endpoint that authenticates a user for testing (endpoint only exists in dev)
    -- create two fixtures
    -- update docs with instructions for running the cypress tests and for using postgres locally (project only had instructions for containerized postgres, which is a pain for testing)
    -- tweak some configs

Additional notes:

  • there are several things I'm unsure about/would love feedback on and I've made line notes

Update Dec 22:

Comment on lines 5 to 9
// clean up db
cy.exec('dropdb -f digitalmarketplace')
// set up db
cy.exec('createdb digitalmarketplace')
cy.exec('npm run migrations:latest;')
Copy link
Collaborator Author

@BCerki BCerki Dec 21, 2021

Choose a reason for hiding this comment

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

I suspect this is inelegant--I would ultimately like to just truncate the tables using something like this, but was having trouble following. For first iteration, I think this is OK?

Choose a reason for hiding this comment

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

Let's track as a tech debt issue? There's the truncate option (per example) and there's also the option of running migrations within a transaction (even faster) depending on how our backend is architected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cy.contains('Get Started').first().click()

// 1. Overview tab
cy.get('#cwu-opportunity-title').type('Cypress Opp')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know IDs aren't the best selectors, but since we're considering a refactor, I didn't want to add data-cy everywhere.

Choose a reason for hiding this comment

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

The recommendations are opinionated. I like https://testing-library.com/ 's philosophy of only relying on things a user can interact with for testing. There's actually a (cypress-testing-library)[https://testing-library.com/docs/cypress-testing-library/intro]! Haven't tried it yet, but this could allow you to write

cy.findByLabelText(/opportunity title/i).type('Cypress Opp')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, thanks! I'll play with it in future tests

Choose a reason for hiding this comment

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

Feature ticket for adding testing-library support? I'll second the opinionated Guiding Principles of that project.

write tests that closely resemble how your web pages are used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I already installed it as part of this PR

// 1. Overview tab
cy.get('#cwu-opportunity-title').type('Cypress Opp')
cy.get('#cwu-opportunity-teaser').type('Teaser text')
cy.get('#cwu-opportunity-remote-ok-0').check({force:true}) // is force OK?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is force OK?

cy.get('#cwu-opportunity-location').clear().type('Vancouver')
cy.get('#cwu-opportunity-reward').type('5000')
cy.get('#cwu-opportunity-skills').click()
cy.contains('Agile').click({force: true})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is in a dropdown and what I've got here works, but it seemed sketchy?

Choose a reason for hiding this comment

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

Ideally we'd wait for the click that opens the dropdown to finish opening, ensuring this click is reachable (so we don't have to force). Tech debt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cy.get('#cwu-opportunity-remote-desc').should('have.value','Remote description text')
cy.get('#cwu-opportunity-location').should('have.value','Vancouver')
cy.get('#cwu-opportunity-reward').should('have.value','5000')
cy.contains('Agile').should('be.visible') // weak
Copy link
Collaborator Author

@BCerki BCerki Dec 21, 2021

Choose a reason for hiding this comment

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

I don't like this selector because the word "Agile" could appear elsewhere depending on the opportunity, so I look forward to your feedback!

Choose a reason for hiding this comment

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

Is this word in a section, or under a label that you could query?

}
},
// /auth/createsession is for testing only, so only exists outside of production
// @ts-ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this is a testing endpoint, not a real one, it doesn't return the types the app expects. OK to use @ts-ignore here and below?

Choose a reason for hiding this comment

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

Ideally we'd cast this endpoint's output to return prod types. Tech debt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@BCerki BCerki marked this pull request as draft December 21, 2021 22:51
@BCerki BCerki mentioned this pull request Dec 22, 2021
@BCerki BCerki changed the title 107 cy login Cypress test to create CWU opportunity Dec 22, 2021
},
// /auth/createsession is for testing only, so only exists outside of production
// @ts-ignore
...(process.env.NODE_ENV === 'development' ? [{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this enough to ensure this endpoint will never exist in prod?

Choose a reason for hiding this comment

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

No, this means that your endpoint will only exist in your development build. When testing with cypress you want to test against your production build.
NODE_ENV is only telling you whether you're looking at a development build (enabling, hot module reloading, react dev tools, etc) or a production build, it is not related to your dev/test/prod environment.

What you want here is an opt-in feature flag, e.g. ENABLE_MOCK_AUTH, and you'll want to run your server with that flag when starting it for your cypress tests (example).

If your application container is aware of which namespace it's running in, e.g. via a NAMESPACE env variable, you could also ensure that this feature flag cannot be enabled when the application is deployed on OpenShift (no matter the namespace).
e.g.

export const ENABLE_MOCK_AUTH = process.argv.includes("ENABLE_MOCK_AUTH") && process.env.NAMESPACE === undefined;

Choose a reason for hiding this comment

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

Yes, I'd say this is safe. Tech debt here is we eventually want a validated configuration (eg. convict, among many others) but this is good for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@BCerki BCerki marked this pull request as ready for review December 22, 2021 19:03
wenzowski
wenzowski previously approved these changes Dec 22, 2021
Copy link

@wenzowski wenzowski left a comment

Choose a reason for hiding this comment

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

Let's document tech debt as issues on the repo and :shipit: shipit

Comment on lines 5 to 9
// clean up db
cy.exec('dropdb -f digitalmarketplace')
// set up db
cy.exec('createdb digitalmarketplace')
cy.exec('npm run migrations:latest;')

Choose a reason for hiding this comment

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

Let's track as a tech debt issue? There's the truncate option (per example) and there's also the option of running migrations within a transaction (even faster) depending on how our backend is architected.

cy.contains('Get Started').first().click()

// 1. Overview tab
cy.get('#cwu-opportunity-title').type('Cypress Opp')

Choose a reason for hiding this comment

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

Feature ticket for adding testing-library support? I'll second the opinionated Guiding Principles of that project.

write tests that closely resemble how your web pages are used

cy.get('#cwu-opportunity-location').clear().type('Vancouver')
cy.get('#cwu-opportunity-reward').type('5000')
cy.get('#cwu-opportunity-skills').click()
cy.contains('Agile').click({force: true})

Choose a reason for hiding this comment

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

Ideally we'd wait for the click that opens the dropdown to finish opening, ensuring this click is reachable (so we don't have to force). Tech debt?

},
// /auth/createsession is for testing only, so only exists outside of production
// @ts-ignore
...(process.env.NODE_ENV === 'development' ? [{

Choose a reason for hiding this comment

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

Yes, I'd say this is safe. Tech debt here is we eventually want a validated configuration (eg. convict, among many others) but this is good for now.

}
},
// /auth/createsession is for testing only, so only exists outside of production
// @ts-ignore

Choose a reason for hiding this comment

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

Ideally we'd cast this endpoint's output to return prod types. Tech debt

@BCerki
Copy link
Collaborator Author

BCerki commented Jan 7, 2022

Work finished in #121

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.

None yet

3 participants