Add tests for static_react_task example

[Etesam913]

Summary

Tests

Configured cypress with the static_react_task webapp.
Added tests that test if:

• a request is made to the "/request_agent" endpoint
• the correct react elements are displayed
• a request is made to the "/submit_task" endpoint after clicking the green button
• a request is made to the "/submit_task" endpoint after clicking the red button
• an alert shows up with the correct text after clicking the green button
• an alert shows up with the correct text after clicking the red button

GitHub Actions

Added a github action workflow named, cypress-e2e-tests which runs the cypress tests as described above.

Linking (RESOLVED)

To make sure that tests on a task adapt when doing local package development, the example task's webapp should be linked to the package.

This can easily be done by writing something like this in the package.json :

"mephisto-task": "file: ../../../packages/mephisto-task",

However, this is not ideal as an individual would have to go to mephisto-task run npm link, and then go to the examples task's webapp folder and run npm link mephisto-task, and then cd out and type python run_task. This is clearly many extra steps.

The best of both worlds would allow someone who is just running the task without doing any local package development work to avoid having to do all the npm linking. This intuitively should work by the following steps:

1. Setting the following in the package.json: "mephisto-task": "^2.0.1",

2. running npm link mephisto-task in the webapp folder

3. Running python run_task.py in the root folder for the task.

However, this is not possible as running python run_task.py runs npm install in the webapp folder (done under the hood), which undoes the link and installs mephisto-task 2.01 from the package.json.

To resolve this issue for local development a new command line arg is defined.

Example:

Step 1:

cd packages/mephisto-task
npm link

Step 2:

cd examples/static_react_task
python run_task.py mephisto.task.package_to_link_to=mephisto-task

This works as this command line arg is read by the program and runs npm link mephisto-task after the npm install to get the local version of the package.

Removing the mephisto.task.package_to_link_to command line arg skips the npm link command. This means that the version of the package as specified in the package.json is used.

This achieves the best of both worlds, easy running of a task when doing no local development, and local development without having to tinker with the package.json by adding a weird file path.

Downsides

https://github.com/facebookresearch/Mephisto/pull/795/files/ac59bbdcde1c861f892d32485aaee87b2c7db8bc#r901965814

Testing

static_react_task_test.mov

To see the GitHub action output you can look at the "cypress-e2e-tests" test and click on "details" in the bottom of this pr.

TODO:

• Planning on adding the information to run these tests in the CONTRIBUTING.md file
• Configure this with GitHub actions
• Look at more Cypress file/test structures to understand if my file structure makes sense

This pr will fix the issues: #787 #788

[codecov-commenter]

Codecov Report

Merging #795 (28c8431) into main (ef1abf9) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #795      +/-   ##
==========================================
+ Coverage   64.25%   64.33%   +0.08%     
==========================================
  Files         107      107              
  Lines        9176     9178       +2     
==========================================
+ Hits         5896     5905       +9     
+ Misses       3280     3273       -7     

Impacted Files	Coverage Δ
mephisto/data_model/task_run.py	84.57% <100.00%> (+0.14%)	⬆️
mephisto/abstractions/architects/mock_architect.py	88.23% <0.00%> (-2.62%)	⬇️
...tractions/architects/channels/websocket_channel.py	76.56% <0.00%> (+8.59%)	⬆️

---

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 ef1abf9...28c8431. Read the comment docs.

[pringshia]

Thanks for describing the issue in the Linking section above. I think I understand the problem here, the Mephisto builder does indeed call npm install and I can see how that would blow out the npm link set up for local development.

I think that adding a config such as mephisto.task.package_to_link_to could work here, though I wonder if we could do something even more generic that could be reusable for more cases in the future and enable flexibility. The current solution seems extremely overfit to the current problem we are facing.

My alternative suggestion here would be something like mephisto.task.post_build_script instead.

---

The usage would then be something like this:

python run_task.py mephisto.task.post_build_script=./link_mephisto_task.sh

and then you have a file link_mephisto_task.sh:

npm link mephisto-task

This way you could run any arbitrary code after the build step. For example, in other dialog tasks we have another local package we use called bootstrap-chat. For that, the post build script would look like:

npm link mephisto-task bootstrap-chat
# here we link two packages instead of just one. running a script allows us this flexibility

---

In sum, I propose introducing "lifecycle hooks"-style config params to allow for more flexible configuration that not only solves for the current problem we have now, but could handle future diverse needs gracefully.

Also see my comment above for how to solve for the optimization issue. We would detect when post_build_script is set and could potentially bypass the optimization in that case?

[Etesam913]

The post_build flag definitely seems like a good suggestion. It allows for much more freedom in choosing what to run.

I'm not sure that checking if post_build_script is not none can be used to sufficiently skip the optimization, however. What I mean is that it doesn't work in both directions.

If the flag does exist then you can skip the optimization and build it. What if you want to go back to the package.json package version and remove your link? Well then you would remove the flag. However, if the webapp is not touched, then no rebuild will be triggered and the linked version will still be used. You would have to make some arbitrary change to the webapp to force the rebuild.

I think the simplest way to do this would be to add a param called force_rebuild that when set to true would skip over the optimization. It would be good for local development and to prevent these weird situations with building.

[pringshia]

I think the simplest way to do this would be to add a param called force_rebuild that when set to true would skip over the optimization. It would be good for local development and to prevent these weird situations with building.

Let's do it. Explicit is always better.

[pringshia]

there a bunch of places we need to update to post install script now

[Etesam913]

Yeah, for some reason I forgot to update it

[Rebecca-Qian]

Looks good - thanks for adding tests!!

[JackUrb]

Will need to execute the update to a 3-param build_custom_bundle as discussed, but overall I'm really excited for this PR. Thanks for getting this together!

We'll definitely want to take the learnings from here to eventually have a test for all of the example tasks, as well as a workflow for being able to create new tests for new tasks (with different configuration options as well like onboarding for instance). Will leave as future work for now though!

[JackUrb]

As a note - we can probably get these to automatically trigger as part of the test suite when any file in packages is updated, such that we get this 'for free' on PRs.

[JackUrb]

Given these have a default hydra value (introduced in TaskRun's file) is it required that we include these defaults in all of these files?

I imagine the tradeoff between discoverability and boilerplate is an important one to think about if not, but I'd lean towards these more landing in the camp of a heavy user or contributor.

[Etesam913]

As most people will not do local package development, I think it makes sense to remove them from the hydra config.

I just tested it, and it works without defining them in the hydra config(uses the TaskRun default value).

[Etesam913]

New warning messages
They link to a closed issue that describes how to fix this

[Etesam913]

An example of passing in the new parameters

[Etesam913]

kwargs is type Union[str, bool] because the parameter value can be a string(post_install_script) or bool(force_rebuild) currently.

[JackUrb]

Shouldn't you be able to just write this as:

def build_custom_bundle(custom_src_dir, force_rebuild: Optional[bool] = None, post_install_script: Optional[str] = None)

[Etesam913]

Yeah, I can do it like that, I was just following Pratik's suggestion here: #791 (comment)

[pringshia]

@Etesam913, you'd still be using kwargs with @JackUrb's suggestion above by naming the arguments in the list

more info: https://treyhunner.com/2018/04/keyword-arguments-in-python/

[Etesam913]

Oh so kwargs just refers to passing the arguments with keywords as opposed to using the **kwargs parameter. I got it 👍

[Etesam913]

#795 (comment)

As a note - we can probably get these to automatically trigger as part of the test suite when any file in packages is updated, such that we get this 'for free' on PRs.

I think I am already doing that in this github action (the start field)

[JackUrb]

Indeed - missed that on first pass!

[pringshia]

Thanks for getting e2e testing finally added to the codebase here