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

OCLOMRS-970:Automated test for creating a version, releasing it and copy subscriptionURL #724

Merged
merged 4 commits into from
Aug 3, 2021

Conversation

suruchee
Copy link
Contributor

JIRA TICKET NAME:

The Loader-Upper

Summary:

Automated test for creating a dictionary version, releasing it, and copy subscription URL

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Thanks for this @suruchee! I've added a bunch of comments on some things to tighten this up a bit, but very good work!

Comment on lines 27 to 31





Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Before({ tags: "@concept" }, () => {
setConceptId(`CT-${nanoid()}`);
});
Before({ tags: "@version" }, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Before({ tags: "@version" }, () => {
Before({ tags: "@version" }, () => {

cy.findByRole("button", { name: /Create new version/i }).click();
});
Then("the user should be on the create new version dialog box", () =>
cy.get("h2").contains("Create new version")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cy.get("h2").contains("Create new version")
cy.findByText("Create new version").should("be.visible")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ibacher, but this code will cause Found multiple elements with the text error. So I tried this one. Will you please suggest another idea?

Copy link
Member

Choose a reason for hiding this comment

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

cy.findByText("Create new version", { selector: "h2" }).should("be.visible")

cy.get("h2").contains("Create new version")
);

Given("the user is on the create new version dialog box", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Given("the user is on the create new version dialog box", () => {
Given("the user clicks on the create new version dialog box", () => {

And a similar change to the feature file.

cy.get("#released").type("{enter}");
});
When("the user submits the form", () => {
cy.get("#versionForm").submit();
Copy link
Member

Choose a reason for hiding this comment

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

There's no #versionForm element, so just do:

cy.findByRole("button", { name: /Submit/i }).click();


@dictionary
@version
Scenario: The user should be able to create a new version, release it and copy subscription URL
Copy link
Member

Choose a reason for hiding this comment

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

I would actually break this out as three separate tests: one to create a new version. One to release it via the checkbox, and one to copy the subscription URL. That way we should also be able to check that the user can release a new version within the "Create New Version" form.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right @ibacher that's what @suruchee had but the last two scenarios were not passing probably because they needed a version to be created, she had a command for creating and getting a version the way the dictionary commands are but they were not working, so I told her that we first put those other two tests in the same scenario so that we see they pass, then we reach out to you about separating them.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough... The version release process actually does some backend processing, so between the request to create the version returning and the version actually being created there's a bit of a delay... not much for a small dictionary, but not no time at all either.

Probably the simplest way to do this is to force Cypress to pause immediately after submitting the new version request, e.g., by using cy.wait(500). A more complicated version of the same idea would be to have a shorter pause and at the end of each pause poll the backend until a longer period of time, e.g. 8 seconds or w/e had passed.

Copy link
Member

Choose a reason for hiding this comment

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

Here's a library to handle my "more complicated" version: https://github.com/NoriSte/cypress-wait-until. Basically looks like you can use that and wait until the backend returns that the version exists. Note that it's probably also necessary to force Cypress to reload the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @hadijahkyampeire and @ibacher, I will update with the delay suggestion.

cy.get('[type="checkbox"]').check();
});
When("the release dialog opens", () => {
cy.get("#confirmation-dialog-title").should("exist");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cy.get("#confirmation-dialog-title").should("exist");
cy.get("#confirmation-dialog-title").should("be.visible");

Comment on lines 41 to 43
cy.get("button.MuiButton-textPrimary")
.eq(1)
.click();
Copy link
Member

Choose a reason for hiding this comment

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

cy.findByText(/Yes/i).click()

cy.findByText(menuItem).click()
);
Then("the subscription url should be copied", () =>
cy.visit(`/users/${getUser()}/collections/${getDictionaryId()}/${getVersionId()}`)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I see how this code checks that the subscription URL was copied? This looks like a good approach to verify that the correct text was copied.

export const getOrganisationId = () => Cypress.env("organisationId");
export const setOrganisationId = (organisationId: string) =>
Cypress.env("organisationId", organisationId);
export const getVersionId = () => Cypress.env("versionId");
export const setVersionId = (versionId: string) =>
Cypress.env("versionId", versionId);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Cypress.env("versionId", versionId);
Cypress.env("versionId", versionId);

@suruchee
Copy link
Contributor Author

Thank you @ibacher, for the review. I have updated the changes but I am not sure about making separate scenarios for release and copy url.

@coveralls
Copy link

coveralls commented Jul 27, 2021

Coverage Status

Coverage remained the same at 46.72% when pulling 974d5ec on suruchee:OCLOMRS-970 into 72e2dfe on openmrs:master.

Copy link
Collaborator

@hadijahkyampeire hadijahkyampeire left a comment

Choose a reason for hiding this comment

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

Very well done @suruchee, I think this will be good to merge once we do the separation with @ibacher 's guidance.

@ibacher ibacher merged commit 976ba59 into openmrs:master Aug 3, 2021
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.

4 participants