Add tests to tips example #833

Etesam913 · 2022-07-17T23:20:07Z

Overview

Adds three cypress end to end tests for the static-react-task-with-tips task example.

0-static_react_task_with_tips_pre_tip_submission.cy
1-static_react_task_with_tips_feedback_submission.cy
static_react_task_with_tips_post_tip_submission.cy

Pre-tip submission

This tests if the tips popup can be opened. It does error checking by typing messages that are too long. It also submits three tips and checks if the server response is correct.

Video:

pre-tip-submission.mp4

Feedback submission

Checks if the correct elements are present and submits answers to the two feedback questions in the example.

Video:

feedback-submission.mov

Post-tip submission

After the pre-tip submission test there are three tips that need to be reviewed. Prior to running the post-tip submission test make sure to run python review_tips_for_task.py and accept the first tip and reject the other two tips.

The test checks to see if the first submitted tip from the pre-tip submission test is present.

Video:

post-tip-submission.mov

GitHub action file

Adds a new job to run the cypress test above.

Installs dependencies and uninstalls detoxify(it is too slow for the GitHub action)
Link to local version of mephisto-task and mephisto-worker-addons packages.
Runs the pre-tip submission cypress test and the feedback_submission cypress test
- The numbers in the file names of these two tests are set the order that the tests are ran in.
Kills the python and node server
- This is supposed to happen automatically after cypress finishes, but this does not happen so I have to do it manually.
The first two units that are launched are expired by running the expire_all_units.py script
- Again, this should happen automatically from shutting down the server, but it doesn't seem to happen in the GitHub action.
- Expiring the units is important because this state is required for the reviewing steps later on.
First and second tips are accepted, while the third tip is rejected.
- This is done by running the review_tips_for_task.py script.
Second tip is removed by running remove_accepted_tip.py.
Reviews the feedback by running review_feedback_for_task.py
The post-tip submission test is ran

ℹ️ It tests if the tips is loaded and can be opened. Then it submits three tips and sees if it gets the correct response.

⏰ Added time to toxicity detection timeout as the test is not passing reliably in the github action

codecov-commenter · 2022-07-17T23:23:07Z

Codecov Report

Merging #833 (c8a673d) into main (7201ee5) will decrease coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #833      +/-   ##
==========================================
- Coverage   64.61%   64.48%   -0.13%     
==========================================
  Files         107      107              
  Lines        9281     9281              
==========================================
- Hits         5997     5985      -12     
- Misses       3284     3296      +12

Impacted Files	Coverage Δ
...tractions/architects/channels/websocket_channel.py	`67.96% <0.00%> (-8.60%)`	⬇️
mephisto/data_model/unit.py	`77.59% <0.00%> (-0.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7201ee5...c8a673d. Read the comment docs.

JackUrb

Overall the test here seems great, thanks for adding it! One note on the added script

JackUrb · 2022-07-20T22:28:36Z

mephisto/scripts/local_db/expire_all_units.py

@@ -0,0 +1,24 @@
+from mephisto.abstractions.databases.local_database import LocalMephistoDB


I feel like this should go in a gh_actions folder

🔀 Merged with main

pringshia

a few comments, but overall LGTM

pringshia · 2022-07-22T21:47:33Z

.github/workflows/cypress-end-to-end-tests.yml

+ pip install -e .
+ pip uninstall detoxify <<EOF
+ y
+ EOF


protip: there's a command called yes that you can use here instead:

yes | pip uninstall detoxify

NAME yes – be repetitively affirmative SYNOPSIS yes [expletive] DESCRIPTION The yes utility outputs expletive, or, by default, “y”, forever.

pringshia · 2022-07-25T18:36:57Z

...bapp/cypress/e2e/pre_submission_tests/0-static_react_task_with_tips_pre_tip_submission.cy.js

+ cy.get('[data-cy="directions-container"]');
+ cy.get('[data-cy="task-data-text"]');
+ cy.get('[data-cy="good-button"]');
+ cy.get('[data-cy="bad-button"]');


i think these could be suffixed with .should('exist') for readability and clearer assertions. this also bakes in some retry semantics...

another option is .should('be.visible') if we want to also check for cases where the element is visible (and not being pushed off screen or hidden by CSS)

that said, i realize that .get by itself does error out if the element isn't found, so perhaps it's not as necessary

Yeah, I don't think it is necessary. I mainly use cy.should('not.exist') to check for content that is not on the page, but I don't really know the point of using cy.should('exist')

yeah it seems it's redundant, it maybe just indicates intention that an assertion is taking place. i'm fine with keeping it as is

pringshia · 2022-07-25T19:33:11Z

mephisto/scripts/local_db/gh_actions/expire_all_units.py

+
+"""
+This script should not be used locally.
+It is only used in a GitHub action as it does not automatically expire units.


this script seems very "one-off"-y. is there no other case we may want to intentionally force expire units? I assume it could be useful? I would consider parametizing the task_name below and having this be a general script?

I'm not aware of any situation outside of the GitHub action where you would want to run a script to expire units. Usually you would expire units by just doing ctrl-c two or three times on your mephisto task.

"Force expiring" units is generally done with a cleanup script, which is provider-associated. This only updates the local status, and is a particular anti-pattern outside of the context of eval scripts.

got it... if this script is really needed for a test, I guess I'm okay with keeping it then

Etesam913 added 5 commits July 17, 2022 19:04

🔧 Configured cypress with tips example

c166229

✨ Added pre tip-submission test

fa03bed

ℹ️ It tests if the tips is loaded and can be opened. Then it submits three tips and sees if it gets the correct response.

✅ Added post tip submission test

de00bec

⏰ Added time to toxicity detection timeout as the test is not passing reliably in the github action

✅ Added feedback submission test

f0762d3

✅ Updated the github action to run the new tests

9ba480c

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 17, 2022

Etesam913 changed the base branch from main to improve-cli-for-feedback-review July 17, 2022 23:20

Etesam913 added 2 commits July 17, 2022 19:25

🔀 Merged with add-feedback-component

4e08d37

✨ Added expire_all_units.py script for action

f95e3c2

Etesam913 changed the title ~~Add tests to tips example~~ [WIP] Add tests to tips example Jul 18, 2022

🔀 Merged with main

9f4c97e

Etesam913 mentioned this pull request Jul 19, 2022

Review outstanding PRs for Cypress integration and the Feedback/Tips component #807

Closed

Etesam913 referenced this pull request Jul 19, 2022

🥅 Improved error message when assets dir is missing

8b8f371

Etesam913 changed the title ~~[WIP] Add tests to tips example~~ Add tests to tips example Jul 20, 2022

Etesam913 requested review from JackUrb and pringshia July 20, 2022 17:41

JackUrb reviewed Jul 20, 2022

View reviewed changes

Etesam913 added 5 commits July 21, 2022 12:35

♻️ Made github action data dir step more concise

748de79

🔀 Merged with main

♻️ Made github action more concise

1aa6a0b

➕ Updated version of mephisto-task in worker-addons

48586a9

➕ Updated dependency in tips example

a69cfc5

➕ Updated yarn.lock

ee75c6b

Etesam913 changed the base branch from improve-cli-for-feedback-review to main July 21, 2022 17:08

✏️ Updated path to expire_all_units.py in action

b2126b8

Etesam913 mentioned this pull request Jul 22, 2022

[Ongoing] Add an e2e integration test #726

Closed

Etesam913 requested a review from JackUrb July 25, 2022 14:28

JackUrb approved these changes Jul 25, 2022

View reviewed changes

pringshia approved these changes Jul 25, 2022

View reviewed changes

♻️ Refactored pip uninstall detoxify command

c8a673d

Etesam913 merged commit 43d40b2 into main Jul 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests to tips example #833

Add tests to tips example #833

Etesam913 commented Jul 17, 2022 •

edited

Loading

codecov-commenter commented Jul 17, 2022 •

edited

Loading

JackUrb left a comment

JackUrb Jul 20, 2022

pringshia left a comment

pringshia Jul 22, 2022

pringshia Jul 25, 2022

pringshia Jul 25, 2022

Etesam913 Jul 25, 2022

pringshia Jul 25, 2022

pringshia Jul 25, 2022

Etesam913 Jul 25, 2022

JackUrb Jul 25, 2022

pringshia Jul 25, 2022

		@@ -0,0 +1,24 @@
		from mephisto.abstractions.databases.local_database import LocalMephistoDB

Add tests to tips example #833

Add tests to tips example #833

Conversation

Etesam913 commented Jul 17, 2022 • edited Loading

Overview

Pre-tip submission

Video:

Feedback submission

Video:

Post-tip submission

Video:

GitHub action file

codecov-commenter commented Jul 17, 2022 • edited Loading

Codecov Report

JackUrb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pringshia left a 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

Choose a reason for hiding this comment

Etesam913 commented Jul 17, 2022 •

edited

Loading

codecov-commenter commented Jul 17, 2022 •

edited

Loading