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

Feature/#93 front end tests #203

Merged
merged 9 commits into from
Feb 1, 2023
Merged

Conversation

leandertolksdorf
Copy link
Collaborator

@leandertolksdorf leandertolksdorf commented Jan 30, 2023

Current test coverage:

Bildschirm­foto 2023-01-30 um 16 25 59

When running cypress tests, the coverage report gets saved into coveragefolder, we could include this in the CI output.

I added more tests for frontend-only stuff like tooltips, but I'm really struggling with tests that involve signals from the backend. See file cypress/e2e/2-indicators/line-indicators.cy.js: I tried mocking the websocket connection to send a test sample array. And the message is received at the frontend. Problems are:

  • The message data is received as a string like 1, 0, 0, ... instead of an array buffer.
  • Despite turning on the oscillator by clicking the start button, $osciEnabled is still false and the onmessage handler doesn't get to updating the other components.

Leander Tolksdorf added 4 commits January 26, 2023 23:33
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
@leandertolksdorf leandertolksdorf marked this pull request as draft January 30, 2023 15:26
@cypress
Copy link

cypress bot commented Jan 30, 2023

Passing run #532 ↗︎

0 42 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge branch 'dev' into feature/#93-front-end-tests
Project: sosci-frontend Commit: 94fa45f44c
Status: Passed Duration: 04:31 💡
Started: Jan 31, 2023 9:48 PM Ended: Jan 31, 2023 9:53 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Collaborator

@PhlppKrmr PhlppKrmr 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 adding all those new tests :)

mockConnection = socket;
});

describe("indicators", () => {
Copy link
Collaborator

@PhlppKrmr PhlppKrmr Jan 31, 2023

Choose a reason for hiding this comment

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

Either change the describe to "line indicators" or maybe even merge the two indicators as before. IMO they are so closeley related that we could even move the two describes into one test file. Would really like the first recommendation. the second is up to your preference :)

cy.get('[data-cy="line-indicators"]').should("be.visible");
});

it("updates min value", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix failing test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Output of cypress test:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what i pointed out in the pr description. Unfortunately after many hours, I coudn't figure out how to fix it so I'm leaving it out for now.

Apps/frontend/src/components/GNDButton.svelte Show resolved Hide resolved
cy.get('[data-cy="gnd-button"]').trigger("mouseover");
cy.get('[data-cy="gnd-button-tooltip"]').should("be.visible");
});
// Stub is not working in the e2e test, createEventDispatcher still
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you suggest we do about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I didn't find a solution to this, I removed it just now and for next sprint we could consider using unit tests for such logic.

@@ -2,7 +2,7 @@

describe("gndButton", () => {
beforeEach(() => {
cy.visit("http://localhost:5173/");
cy.visit("http://localhost:5173/", {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we introduce {} here?

cy.visit("http://localhost:5173/");
});
it("resets on off button when reset button is clicked", () => {
cy.get('[data-cy="on-off-button"]').click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't specifically need to click the on-off-button, but I like that you introduce it that way :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what you meant, but to test if the reset button resets the on off button, I have to first toggle the on off button, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I just meant functionality-wise you would not actually need the extra step. When considering the label of the test it is even expected that you also click the button. My misunderstaning

@@ -17,4 +22,10 @@ describe("everything visible on front page", () => {
cy.get('[data-cy="selected-preset"]').should("be.visible");
cy.get('[data-cy="store-channel-config"]').should("be.visible");
});

it("closes popup on click outside", () => {
cy.get('[data-cy="settings-button"]').click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would like it if we also checked if the setting panel is open after clicking the settings-button, because if the button click never opens it the test will still succeed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell the interim step of checking if the panel is/was open is missing.
cy.get('[data-cy="preset-config-open-popup"]').should("be.visible");

Apps/frontend/cypress.config.js Show resolved Hide resolved
Leander Tolksdorf added 4 commits January 31, 2023 22:18
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
Signed-off-by: Leander Tolksdorf <leander.tolksdorf@fu-berlin.de>
@leandertolksdorf leandertolksdorf marked this pull request as ready for review January 31, 2023 21:39
Copy link
Collaborator

@motschel123 motschel123 left a comment

Choose a reason for hiding this comment

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

All tests running green, LGTM

@@ -17,4 +22,10 @@ describe("everything visible on front page", () => {
cy.get('[data-cy="selected-preset"]').should("be.visible");
cy.get('[data-cy="store-channel-config"]').should("be.visible");
});

it("closes popup on click outside", () => {
cy.get('[data-cy="settings-button"]').click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell the interim step of checking if the panel is/was open is missing.
cy.get('[data-cy="preset-config-open-popup"]').should("be.visible");

cy.visit("http://localhost:5173/");
});
it("resets on off button when reset button is clicked", () => {
cy.get('[data-cy="on-off-button"]').click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I just meant functionality-wise you would not actually need the extra step. When considering the label of the test it is even expected that you also click the button. My misunderstaning

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.

3 participants