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

Add tests for checking Unsaved changes modal #19080

Merged
merged 8 commits into from
Nov 14, 2022

Conversation

SofiiaZaitseva
Copy link
Contributor

Add tests for checking Unsaved changes modal:

  1. User opened the New source page and leaves the page(or any other pages) - should NOT see the unsaved changes modal
  2. User opened the New source page and selected one of the connectors and leaves the page(or any other pages) - should NOT see the unsaved changes modal
  3. User opened the New source page and selected one of the connectors, “touched” one of the fields and leaves the page(or any other pages) - should see the unsaved changes modal
  4. User opened the New source => selected connector => filled in the fileds => start testing. If it was successful it should redirected to Connector Overview page
  5. User opened the New source => selected connector => filled in the fileds => start testing. If it was failed and user tries to leave the page - should see the unsaved changes modal

@github-actions github-actions bot removed the area/frontend Related to the Airbyte webapp label Nov 7, 2022
@SofiiaZaitseva SofiiaZaitseva marked this pull request as ready for review November 7, 2022 19:16
@SofiiaZaitseva SofiiaZaitseva requested a review from a team as a code owner November 7, 2022 19:16
@@ -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

Comment on lines 96 to 98
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!

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

Comment on lines 20 to 23
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.


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


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

Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

Did a quick review, agree with @ambirdsall comments - need to polish a few things.

@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 9, 2022
Copy link
Contributor

@ambirdsall ambirdsall left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, just one last request.

Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍
All comments are resolved.
CI tests run- ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform team/compose
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants