Add tests for checking Unsaved changes modal #19080

SofiiaZaitseva · 2022-11-07T17:09:34Z

Add tests for checking Unsaved changes modal:

User opened the New source page and leaves the page(or any other pages) - should NOT see the unsaved changes modal
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
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
User opened the New source => selected connector => filled in the fileds => start testing. If it was successful it should redirected to Connector Overview page
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

ambirdsall · 2022-11-07T23:03:01Z

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts

 });
+
+describe("Unsaved changes modal", () => {
+ beforeEach(() => {


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);

I used beforeEach(() => initialSetupCompleted()); due to initialSetupCompleted has optional parameter and cannot be used in this beforeEach(initialSetupCompleted); way

ambirdsall · 2022-11-08T00:11:49Z

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts

+ 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']")


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:

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>

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.

fixed, thanks!

ambirdsall · 2022-11-08T07:43:14Z

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts

+import { openHomepage } from "pages/sidebar";
+import { selectServiceType } from "pages/createConnectorPage";
+import { fillPokeAPIForm } from "commands/connector";
+import { should } from "chai";


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

ambirdsall · 2022-11-08T08:29:06Z

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts

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


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}`); });

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.

ambirdsall · 2022-11-08T08:30:29Z

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts

+
+ openHomepage();
+
+ cy.url().should("include", `/onboarding`);


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

ambirdsall · 2022-11-08T08:31:11Z

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts

+
+ openHomepage();
+
+ cy.url().should("include", `/onboarding`);


Consistent quote marks again

dizel852

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

…rbytehq/airbyte into Sofiia_CheckUnsavedModalTests

ambirdsall

Mostly looks good to me, just one last request.

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts

dizel852

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

add tests

add tests

05e1cb9

SofiiaZaitseva added area/frontend team/compose area/frontend Related to the Airbyte webapp labels Nov 7, 2022

github-actions bot removed the area/frontend Related to the Airbyte webapp label Nov 7, 2022

SofiiaZaitseva added 3 commits November 7, 2022 19:10

Merge branch 'master' into Sofiia_CheckUnsavedModalTests

c8f9c7d

Merge branch 'master' into Sofiia_CheckUnsavedModalTests

d65c8a5

add comment

14093b4

SofiiaZaitseva marked this pull request as ready for review November 7, 2022 19:16

SofiiaZaitseva requested a review from a team as a code owner November 7, 2022 19:16

SofiiaZaitseva requested review from ambirdsall and dizel852 November 7, 2022 19:16

Merge branch 'master' into Sofiia_CheckUnsavedModalTests

2985c27

ambirdsall reviewed Nov 8, 2022

View reviewed changes

dizel852 requested changes Nov 8, 2022

View reviewed changes

SofiiaZaitseva added 2 commits November 9, 2022 20:24

fix comments

0637191

Merge branch 'Sofiia_CheckUnsavedModalTests' of https://github.com/ai…

921148d

…rbytehq/airbyte into Sofiia_CheckUnsavedModalTests

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

ambirdsall reviewed Nov 10, 2022

View reviewed changes

airbyte-webapp-e2e-tests/cypress/integration/source.spec.ts Show resolved Hide resolved

add additional check

972f211

dizel852 approved these changes Nov 10, 2022

View reviewed changes

SofiiaZaitseva merged commit 065cf66 into master Nov 14, 2022

SofiiaZaitseva deleted the Sofiia_CheckUnsavedModalTests branch November 14, 2022 16:32

github-actions bot mentioned this pull request Nov 15, 2022

Bump helm chart version reference to 0.40.43 #19453

Closed

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

akashkulk pushed a commit that referenced this pull request Dec 2, 2022

Add tests for checking Unsaved changes modal (#19080)

0a6388d

add tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add tests for checking Unsaved changes modal #19080

Add tests for checking Unsaved changes modal #19080

Uh oh!

SofiiaZaitseva commented Nov 7, 2022

ambirdsall Nov 7, 2022

SofiiaZaitseva Nov 9, 2022

ambirdsall Nov 8, 2022

SofiiaZaitseva Nov 9, 2022

ambirdsall Nov 8, 2022

SofiiaZaitseva Nov 9, 2022

ambirdsall Nov 8, 2022

dizel852 Nov 8, 2022

ambirdsall Nov 8, 2022

ambirdsall Nov 8, 2022

dizel852 left a comment

ambirdsall left a comment

Uh oh!

dizel852 left a comment

Labels

4 participants

Add tests for checking Unsaved changes modal #19080

Add tests for checking Unsaved changes modal #19080

Uh oh!

Conversation

SofiiaZaitseva commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dizel852 left a comment

Choose a reason for hiding this comment

ambirdsall left a comment

Choose a reason for hiding this comment

Uh oh!

dizel852 left a comment

Choose a reason for hiding this comment

Labels

4 participants