Skip to content

Add tests for checking Unsaved changes modal #19080

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Nov 14, 2022
Merged

Add tests for checking Unsaved changes modal #19080

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
05e1cb9
add tests
SofiiaZaitseva Nov 7, 2022
c8f9c7d
Merge branch 'master' into Sofiia_CheckUnsavedModalTests
SofiiaZaitseva Nov 7, 2022
d65c8a5
Merge branch 'master' into Sofiia_CheckUnsavedModalTests
SofiiaZaitseva Nov 7, 2022
14093b4
add comment
SofiiaZaitseva Nov 7, 2022
2985c27
Merge branch 'master' into Sofiia_CheckUnsavedModalTests
SofiiaZaitseva Nov 7, 2022
0637191
fix comments
SofiiaZaitseva Nov 9, 2022
921148d
Merge branch 'Sofiia_CheckUnsavedModalTests' of https://github.com/ai…
SofiiaZaitseva Nov 9, 2022
972f211
add additional check
SofiiaZaitseva Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 69 additions & 2 deletions airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,26 @@
import { appendRandomString } from "commands/common";
import { appendRandomString, submitButtonClick } from "commands/common";
import { createPostgresSource, deleteSource, updateSource } from "commands/source";
import { initialSetupCompleted } from "commands/workspaces";
import { goToSourcePage, openNewSourceForm } from "pages/sourcePage";
import { openHomepage } from "pages/sidebar";
import { selectServiceType } from "pages/createConnectorPage";
import { fillPokeAPIForm } from "commands/connector";
import { should } from "chai";
Copy link
Contributor

Choose a reason for hiding this comment

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

This { should } import is unused, and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed



describe("Source main actions", () => {
beforeEach(() => {
initialSetupCompleted();
});

it("Create new source", () => {
cy.intercept("/api/v1/sources/create").as("createSource");
createPostgresSource("Test source cypress");

cy.url().should("include", `/source/`);
cy.wait("@createSource", {timeout: 30000}).then((interception) => {
assert("include", `/source/` + interception.response?.body.Id)});

//cy.url().should("include", `/source/`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does

assert("include", `/source/${interception.response?.body.Id}`)`

assert against the url? (As an aside, if you're using backticks, you can insert values into the string with code, like ${value}, which is nicer to read) It would be a bit more readable to keep the old assertion style:

cy.wait("@createSource", { timeout: 30000 }).then((interception) => {
  cy.url().should("include", `/source/${interception.response?.body.Id}`);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

assert against the url?

Yeah, that's actually my suggestion during short sync with @SofiiaZaitseva. After connector creation, we aim to check that we were redirected to the "Overview" page.

});

//TODO: add update source on some other connector or create 1 more user for pg
Expand All @@ -32,3 +42,60 @@ describe("Source main actions", () => {
cy.get("div").contains(sourceName).should("not.exist");
});
});

describe("Unsaved changes modal", () => {
beforeEach(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor detail: there's no need for the anonymous wrapper function (() => { initialSetupCompleted() }) here. All describe needs for its second argument is some function that, when called with no arguments, does the correct setup, and that's exactly what the initialSetupCompleted value is:

// all three of these do the same thing in the context of a test
beforeEach(() => {
  initialSetupCompleted();
})

// and these last two behave exactly the same in every way
beforeEach(() => initialSetupCompleted());

// but this one is the shortest and cleanest
beforeEach(initialSetupCompleted);

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 used beforeEach(() => initialSetupCompleted()); due to initialSetupCompleted has optional parameter and cannot be used in this beforeEach(initialSetupCompleted); way

initialSetupCompleted();
});

it("Check leaving Source page without any changes", () => {
goToSourcePage();
openNewSourceForm();

openHomepage();

cy.url().should("include", `/onboarding`);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use the same style of quotes throughout, unless you're using the interpolation feature of the backtick strings

});

it("Check leaving Source page without any changes after selection type", () => {
goToSourcePage();
openNewSourceForm();
selectServiceType("PokeAPI");

openHomepage();

cy.url().should("include", `/onboarding`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistent quote marks again

});

it("Check leaving Source page without any changes", () => {
goToSourcePage();
openNewSourceForm();
fillPokeAPIForm("testName", "ditto");

openHomepage();

cy.get("#headlessui-portal-root h5").should("exist");
cy.get("#headlessui-portal-root h5").should("have.text", "Discard changes");
cy.get("#headlessui-portal-root [class*='ConfirmationModal_content']")
.should("have.text", "There are unsaved changes. Are you sure you want to discard your changes?CancelDiscard changes");
});

//BUG - https://github.com/airbytehq/airbyte/issues/18246
it.skip("Check leaving Source page after failing testing", () => {
cy.intercept("/api/v1/scheduler/sources/check_connection").as("checkSourceUpdateConnection");

goToSourcePage();
openNewSourceForm();
fillPokeAPIForm("testName", "name");
submitButtonClick();

cy.wait("@checkSourceUpdateConnection", {timeout: 5000});

openHomepage();

cy.get("#headlessui-portal-root h5").should("exist");
cy.get("#headlessui-portal-root h5").should("have.text", "Discard changes");
cy.get("#headlessui-portal-root [class*='ConfirmationModal_content']")
Copy link
Contributor

Choose a reason for hiding this comment

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

These selectors are a bit brittle: if we ever stop using the headlessui library for this modal or use a different level heading than h5, the test will break even if the page behavior remains correct.

In this case, I would make the following changes:

  1. add a data-testid attribute to the confirmation modal component (I believe this is in airbyte-webapp/src/components/common/ConfirmationModal/ConfirmationModal.tsx, but I have not double-checked that). Adding it to the outermost Modal component should be fine:
<Modal onClose={onClose} title={<FormattedMessage id={title} />} data-testid="confirmationModal">
  {/* rest of modal implementation */}
</Modal>
  1. use that testid attribute in the test. To avoid having to be overly specific with your selectors, use cy.get("[data-testid='confirmationModal']").contains(text) for all assertions, e.g. cy.get("[data-testid='confirmationModal']").contains("Discard changes") or cy.get("[data-testid='confirmationModal']").contains("There are unsaved changes. Are you sure you want to discard your changes?"). Since cy.contains returns true if the text is anywhere within the component, this approach lets our test code remain blissfully unaware of any future changes to the structure or implementation of the modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks!

.should("have.text", "There are unsaved changes. Are you sure you want to discard your changes?CancelDiscard changes");
});
});
5 changes: 5 additions & 0 deletions airbyte-webapp-e2e-tests/cypress/pages/sidebar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
const setting = "nav a[href*='settings']";
const homepage = "[aria-label='Homepage']";

export const openSettings = () => {
cy.get(setting).click();
};

export const openHomepage = () => {
cy.get(homepage).click();
};