fossasia / visdom

A flexible tool for creating, organizing, and sharing visualizations of live, rich data. Supports Torch and Numpy.
Apache License 2.0
10.04k stars 1.13k forks source link

Testing using cypress #850

Closed da-h closed 2 years ago

da-h commented 2 years ago

Description

This PR implements a baseline for automated testing of visdom using cypress.

Motivation and Context

Following the discussion in #849, this PR introduces some basic tests for many features of visdom.

In more detail:

Additionally:

How Has This Been Tested?

Who tests the tester? ;)

If you want to try it, do:

  1. start a visdom server on port 8098
  2. run npm run test:gui
  3. click on screenshots.init.js (otherwise the visual regression tests in screenshots.js will fail)

Screenshots (if appropriate):

Types of changes

Checklist:

JackUrb commented 2 years ago

WOW I love the cypress testing suite here. Huge kudos for getting it set up! I'd love to get this into CI for the project in some form, which I know @mariobehling was hoping we could add at some point.

I think I'm running into some issues though, and I'm not sure quite why: Some tests fail with missing content Screen Shot 2022-04-21 at 4 12 57 PM And others aren't getting the exact value they're looking for Screen Shot 2022-04-21 at 4 13 57 PM but most frequently tests fail because "Timed out retrying after 4000ms: Expected to find element: .rc-tree-select-selection__clear, but never found it.", but I think that has to do with a cascading failure after the env window isn't closed.

da-h commented 2 years ago

Too bad I got sloppy at the end. Sorry for the rough mistakes. -.-

Please check my resubmission above. :)

There were a few places that needed to be revisited:

da-h commented 2 years ago

In total, these are the issues I found during testing or / and which I will file in next week in the issue tracker:

da-h commented 2 years ago

Here are a few more notes:

da-h commented 2 years ago

Unfortunately there were still two small bugs in it. I have to admit, this whole PR was much more fiddly than I expected. :)

  1. The first problem that occurred especially with a completely fresh server instance was that the tree-select structure is not loaded expanded if a new folder is created by an env update. As far as I can see this is a bug of TreeSelect, because this actually gets the argument treeDefaultExpandAll. I did not see this problem before, as I didn't restart the server too often during cypress testing. I solved the problem by having the test script open all closed folders in the selection.
  2. The second problem occurred during CI testing. The x_download tests got inconsistent results. I guess this is due to varying download speeds / buffering speeds.
    I removed these tests from the test-list.

I will repush the CI and the changed files later today.

JackUrb commented 2 years ago

I have to admit, this whole PR was much more fiddly than I expected

No worries @da-h - you took on a pretty impressive set of changes rather quickly. I'm always happy to review and help - there's no rush!

da-h commented 2 years ago

Here are the most recent changes:

The new workflow "process-changes" does the following:

  1. build & install everything based on the proposed PR state
    • in doing so, it also commits the build files, if any *.js files changed
  2. each sub-step uses an action prepare to download / or load cached data to speed-up the actions overall. (caching of pip and npm downloads)
  3. the visual regression test consists of two steps: first it uses the base branch to create screenshots (in the future this could be cached as well), the next step compares the same screenshots with that of the pr-branch
  4. in parallel to the visual regression test, there is a functional cypress test

In total, the process looks like this: image

I have tested the new CI using these three PRs

da-h commented 2 years ago

It seems there is one last error in fetching from a feature branch if $GITHUB_HEAD_REF is coming from a fork (instead of origin).

da-h commented 2 years ago

As far as I can tell, this PR is now ready for the detailed review. Sorry again for the constant back and forth. As I said, this PR was quite a bumpy ride after all.

The last two commits change the process a bit, introducing the committing of build files in a seperate job. image

With one exception, the errors in the PR-checks below are only due to missing files on master that are introduced in this very PR. As shown in the example PRs these errors do not show up in succeeding PRs.
The remaining error in the functional test is a bit mysterious as I cannot reproduce it, neither on my machine, nor on the PR-tests linked above.

da-h commented 2 years ago

As a final test, I tried to test against the changes in #849. (See the respective test-PR in my own fork)

Unfortunately, two CI-runs of the same pr yield inconsistent results, while it passes locally on my maschine.

See:

da-h commented 2 years ago

After having read a bit about other people complaining about inconsistent cypress results, especially in github action runs (e.g. here), i think I have finally found the solution for the problem. It was a user error -- what else.

First, & just for future reference. I have tested many suggestions (from the links above) without success:

The error was:

I set the PR thus again on ready, and turn again at the CI-failure-success wheel of fortune. ;) The argument for a (this time) truly finished PR are the following consistent results of the functional tests here, here and here.

da-h commented 2 years ago

PS: As explained above, the visual regression tests are expected to fail for now due to missing files in the current master branch.

da-h commented 2 years ago

In the newest revision, I reverted to the original workflow update-js-build-files.yml.

I had hoped to merge the two workflows, but it is not so easy or at least not intended to commit to the original branch automatically when a PR is based on a branch of a fork. So the current suggestion is to leave the static-build-files-bot as it was (i.e. push-events initiate the bot on a per-branch basis) and additionally run the cypress tests on an incoming PR.

The advantage of all these additional tests is at least that the cypress CI tests have now been run several more times and i am now more sure that they are consistent. :)

da-h commented 2 years ago

🥳