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

Revise actions test #586

Merged
merged 2 commits into from
Sep 9, 2021
Merged

Revise actions test #586

merged 2 commits into from
Sep 9, 2021

Conversation

petterwildhagen
Copy link
Contributor

@petterwildhagen petterwildhagen commented Sep 1, 2021

Initial state in database is set in beforeEach for each test.
The the test data used in the test is retrieved or created in the test itself.

@hnformentin

This comment has been minimized.

@hnformentin
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@petterwildhagen
Copy link
Contributor Author

petterwildhagen commented Sep 3, 2021

Regarding commit messages, not sure if there are any guidelines - but I fixed the commit message so it is a bit clearer. @hnformentin

frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
seed.plant().then(() => {
const user = faker.random.arrayElement(seed.participants).user
cy.visitEvaluation(seed.evaluationId, user)
function checkActionsPresentOnProgression() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that Present would be more specific if substituted by Exist or AreVisible, depending on what you decide for the comment below.

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 think either is fine. Exist, Present, are visible are all generally understood and the same thing in this context

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there was a difference: exist meaning that the element exists in the DOM (even if not rendered), be.visible if the element is rendered. I did not find more than https://filiphric.com/cypress-basics-check-if-element-exists to explain, so fine until I find out more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a user perspective they are the same. Is the action visible to the user, then it is also visible and exists (also in the dom). The e2e tests should always be written from a users perspective.

@hnformentin

This comment has been minimized.

@petterwildhagen
Copy link
Contributor Author

petterwildhagen commented Sep 7, 2021

@hnformentin to your comment above, I organized the tests in different contexts:
actions2

@petterwildhagen petterwildhagen force-pushed the revise_actions_test branch 3 times, most recently from 6b945e0 to bf1917f Compare September 7, 2021 10:43
@denektenina
Copy link
Contributor

I like the newest organization better too :)

@ErlendHaa
Copy link
Contributor

Regarding commit messages, not sure if there are any guidelines - but I fixed the commit message so it is a bit clearer.

https://chris.beams.io/posts/git-commit/

frontend/cypress/integration/actions_spec.ts Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
frontend/cypress/integration/actions_spec.ts Outdated Show resolved Hide resolved
@denektenina
Copy link
Contributor

All my comments have been addressed and the overall structure change seems good to me. The failing tests should be fixed before merge.

Copy link
Contributor

@denektenina denektenina left a comment

Choose a reason for hiding this comment

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

Make sure failing tests run before merging

@petterwildhagen
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@hnformentin

This comment has been minimized.

@@ -174,16 +155,15 @@ describe('Actions', () => {
}
})

cy.login(seed.participants[0].user)
deleteAction()
deleteAction(actionToDelete)
Copy link
Contributor

Choose a reason for hiding this comment

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

@petterwildhagen petterwildhagen force-pushed the revise_actions_test branch 3 times, most recently from f82b74e to c65d2c5 Compare September 9, 2021 09:20
@petterwildhagen
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@petterwildhagen petterwildhagen force-pushed the revise_actions_test branch 3 times, most recently from a0c97f1 to e5d8b47 Compare September 9, 2021 10:48
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.

Cypress tests: Action tests: Separate testdata from setting of initial state
4 participants