e2e upgrade #2352

ignapas · 2021-05-27T09:31:42Z

What do these changes do?

Uses a combination of puppeteer, jest-puppeteer and jest that appears to be more stable. Tests seem to run a bit more consistent across platforms (MacOS, Linux, etc) with more consistent results. And screenshots don't hang.
Also checks for file names instead of only number of files.
Adds a different, not GUI method to delete the studies.
When puppeteer waits, it is possible to include a reason that will be logged.
Flash messages are logged

Related issue/s

How to test

Checklist

codecov · 2021-05-27T09:32:49Z

Codecov Report

Merging #2352 (4413e43) into master (bb62cf8) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #2352     +/-   ##
========================================
- Coverage    74.8%   74.7%   -0.1%     
========================================
  Files         516     516             
  Lines       20033   20033             
  Branches     1971    1971             
========================================
- Hits        14986   14974     -12     
- Misses       4530    4538      +8     
- Partials      517     521      +4

Flag	Coverage Δ
integrationtests	`67.3% <ø> (-0.1%)`	⬇️
unittests	`68.2% <ø> (-0.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...e_director_v2/modules/db/repositories/comp_runs.py	`88.3% <0.0%> (-4.7%)`	⬇️
...c/simcore_service_director_v2/modules/scheduler.py	`93.5% <0.0%> (-3.3%)`	⬇️
...ages/service-library/src/servicelib/aiopg_utils.py	`87.5% <0.0%> (-3.2%)`	⬇️
...webserver/computation_comp_tasks_listening_task.py	`84.0% <0.0%> (-2.0%)`	⬇️
.../director/src/simcore_service_director/producer.py	`60.8% <0.0%> (-0.3%)`	⬇️

GitHK

👍 just a question below

GitHK · 2021-06-01T10:30:45Z

tests/e2e/tutorials/isolve-gpu.js

  }
  catch(err) {
    tutorial.setTutorialFailed(true);
    console.log('Tutorial error: ' + err);
  }
  finally {
+    await tutorial.toDashboard();
+    await tutorial.removeStudy2(studyId);


I see the purpose of removeStudy2. Why don't you rename it and move the call to the catch section? (and keep the original way to removeStudy)
Also, removeStudy2 doesn't need to go toDashboard

agree with @odeimaiz, then we could detect when the remove study fails and still remove the studies

GitHK · 2021-06-01T10:31:43Z

tests/e2e/tutorials/tutorialBase.js

    await utils.sleep(waitFor);
+    await this.takeScreenshot('waitFor_finished')


GitHK · 2021-06-01T10:32:13Z

tests/e2e/utils/startPuppe.js

+    width: 1680,
+    height: 950


why was this done?

it was too big for my laptop screen to check it in demo mode

odeimaiz

As I said, I would not remove the screenshooter.
removeStudy2 uses Resources instead of interacting with the GUI for deleting a study. What's the benefit?
Checking file names is only done in 3 out of 5 tutorials, why?

ignapas · 2021-06-01T12:27:55Z

As I said, I would not remove the screenshooter.

removeStudy2 uses Resources instead of interacting with the GUI for deleting a study. What's the benefit?

Checking file names is only done in 3 out of 5 tutorials, why?

I will make the screenshooter work only during waiting time
Using the Resources class makes it more resilient as it can always remove the study, no matter the state of the interface. Before the studies were not removed because the test was failing half way or it was not possible to go back to the Dashboard.
I will check the remaining two tutorials, my bad

sanderegg

very nice!
please just consider the issue with the Kember test

sanderegg · 2021-06-02T05:39:55Z

tests/e2e/tutorials/mattward.js

    console.log("Study ID:", studyId);

    const workbenchData = utils.extractWorkbenchData(studyData["data"]);
    await tutorial.waitForServices(workbenchData["studyId"], [workbenchData["nodeIds"][0]]);

    // Wait for the output files to be pushed
-    await tutorial.waitFor(30000);
+    await tutorial.waitFor(30000, 'Wait for the output files to be pushed');


nice, probably then we can remove the duplicated comments.

tests/e2e/tutorials/tutorialBase.js

sanderegg · 2021-06-02T05:43:57Z

tests/e2e/tutorials/tutorialBase.js

+  async removeStudy2(studyId) {
+    console.log(`Removing study ${studyId}`)
+    const resp = await this.__page.evaluate(async function(studyId) {
+      return await osparc.data.Resources.fetch('studies', 'delete', {


sanderegg · 2021-06-02T05:45:49Z

tests/e2e/tutorials/isolve-gpu.js

  }
  catch(err) {
    tutorial.setTutorialFailed(true);
    console.log('Tutorial error: ' + err);
  }
  finally {
+    await tutorial.toDashboard();
+    await tutorial.removeStudy2(studyId);


agree with @odeimaiz, then we could detect when the remove study fails and still remove the studies

pcrespov

What is the drawback of having continues screenshots that you decided to remove it? I also find them very useful to debug (together with the client's log) and since the last update they do not take a lot of space anymore.

ignapas · 2021-06-03T09:12:01Z

What is the drawback of having continues screenshots that you decided to remove it? I also find them very useful to debug (together with the client's log) and since the last update they do not take a lot of space anymore.

imo is not that they are bad themselves.. even if there are some things I don't like, like most of them being useless, or the periodicity, that seems a bit arbitrary. the same with the waiting times. also, we would like to know what went wrong just by reading a descriptive log rather than searching and opening some screenshots and wondering what happened of course based on a deep knowledge about the platform. By disabling them, I am more trying to force ourselves to do it differently.

for example: instead of taking one screenshot every two seconds, just take screenshots when they are needed: before and after a click or action is performed, or whenever a notification appears on the screen (although this could be just logged, I will try that). instead of waiting for a fixed amount of time for stuff to happen, just wait for the DOM to change in some way. @odeimaiz what do you think?

odeimaiz · 2021-06-03T09:35:13Z

What is the drawback of having continues screenshots that you decided to remove it? I also find them very useful to debug (together with the client's log) and since the last update they do not take a lot of space anymore.

imo is not that they are bad themselves.. even if there are some things I don't like, like most of them being useless, or the periodicity, that seems a bit arbitrary. the same with the waiting times. also, we would like to know what went wrong just by reading a descriptive log rather than searching and opening some screenshots and wondering what happened of course based on a deep knowledge about the platform. By disabling them, I am more trying to force ourselves to do it differently.

for example: instead of taking one screenshot every two seconds, just take screenshots when they are needed: before and after a click or action is performed, or whenever a notification appears on the screen (although this could be just logged, I will try that). instead of waiting for a fixed amount of time for stuff to happen, just wait for the DOM to change in some way. @odeimaiz what do you think?

I agree that a better logging would help, but I still think the optimization step is too far. I would keep the screenshooter as is.

Conflicts: tests/e2e/package.json

pcrespov

Some recommendations on the PR:

Add a more descriptive title. It helps us when we build release notes
Assign the case to yourself
Assign this sprint as Milestone
Add labels to classify the PR
Comment and resolve all reviews (now there is a new feature "Conversations")

odeimaiz

removeStudy2: rename it and move it to the catch section
Leave the screenshooter as is. If you want to start adding logs that eventually will make the every-2"-screenshot useless, very good, but in the meantime I wouldn't touch it.

ignapas · 2021-06-10T13:41:51Z

removeStudy2: rename it and move it to the catch section

Leave the screenshooter as is. If you want to start adding logs that eventually will make the every-2"-screenshot useless, very good, but in the meantime I wouldn't touch it.

done

odeimaiz · 2021-06-11T06:56:31Z

You win....

pcrespov

👍

ignapas added 3 commits May 20, 2021 13:47

merging

0d0c1aa

mattward seems to work

2e41616

improved mattward sequence and some common methods

b27f950

ignapas added 5 commits May 27, 2021 11:34

Merge branch 'master' into e2e

e564e4a

sleepers working

71dffe3

isolvegpu adapted

2c531bb

Merge branch 'master' into e2e

873a321

Merge branch 'e2e' of github.com:ignapas/osparc-simcore into e2e

afc9030

ignapas marked this pull request as ready for review June 1, 2021 10:00

ignapas requested review from odeimaiz, sanderegg, pcrespov and GitHK June 1, 2021 10:00

GitHK approved these changes Jun 1, 2021

View reviewed changes

odeimaiz requested changes Jun 1, 2021

View reviewed changes

sanderegg reviewed Jun 2, 2021

View reviewed changes

pcrespov reviewed Jun 2, 2021

View reviewed changes

ignapas added 6 commits June 3, 2021 09:21

fixed jupyters tutorial and always deleteing study

d59b177

Merge branch 'master' into e2e

a613ef1

fixed every tutorial

0a1e412

Decrease screenshooter freq

a8e7224

modified p2e tests

bc7642f

remove study goes to dashboard first

28e9b01

making check number of files optional

f0321af

added notification logger

e916094

ignapas requested a review from sanderegg June 3, 2021 14:19

ignapas requested review from odeimaiz and pcrespov June 3, 2021 14:19

Merge branch 'master' into e2e

e3e8ced

Conflicts: tests/e2e/package.json

pcrespov reviewed Jun 3, 2021

View reviewed changes

ignapas changed the title ~~some e2e~~ e2e upgrade Jun 4, 2021

ignapas self-assigned this Jun 4, 2021

ignapas added this to the Chinchilla milestone Jun 4, 2021

ignapas added the e2e Bugs found by or related to the end-2-end testing label Jun 4, 2021

odeimaiz requested changes Jun 7, 2021

View reviewed changes

ignapas added 5 commits June 10, 2021 13:53

Merge branch 'master' into e2e

14f77e8

removeStudy2 doesnt go to dashboard and waitFor takes no screenshots

e0848b9

added screenshooter

06e9b07

Added standard study deletion as backup of interactive one

256e997

Removed duplicated comments and using previous removeStudy

ede6329

ignapas requested review from odeimaiz and pcrespov June 10, 2021 13:42

odeimaiz approved these changes Jun 11, 2021

View reviewed changes

Merge branch 'master' into e2e

8c67625

pcrespov reviewed Jun 11, 2021

View reviewed changes

Merge branch 'master' into e2e

4413e43

ignapas merged commit a18f686 into ITISFoundation:master Jun 15, 2021

sanderegg modified the milestones: Chinchilla, Marmoset Jun 30, 2021

sanderegg mentioned this pull request Jun 30, 2021

maintenance/scaling of the platform ITISFoundation/osparc-issues#428

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e upgrade #2352

e2e upgrade #2352

ignapas commented May 27, 2021 •

edited

Loading

codecov bot commented May 27, 2021 •

edited

Loading

GitHK left a comment

GitHK Jun 1, 2021

odeimaiz Jun 1, 2021 •

edited

Loading

sanderegg Jun 2, 2021

GitHK Jun 1, 2021

GitHK Jun 1, 2021

ignapas Jun 1, 2021

odeimaiz left a comment

ignapas commented Jun 1, 2021

sanderegg left a comment

sanderegg Jun 2, 2021

sanderegg Jun 2, 2021

sanderegg Jun 2, 2021

pcrespov left a comment

ignapas commented Jun 3, 2021

odeimaiz commented Jun 3, 2021 •

edited

Loading

pcrespov left a comment •

edited

Loading

odeimaiz left a comment •

edited

Loading

ignapas commented Jun 10, 2021

odeimaiz commented Jun 11, 2021

pcrespov left a comment

		await utils.sleep(waitFor);
		await this.takeScreenshot('waitFor_finished')

e2e upgrade #2352

e2e upgrade #2352

Conversation

ignapas commented May 27, 2021 • edited Loading

What do these changes do?

Related issue/s

How to test

Checklist

codecov bot commented May 27, 2021 • edited Loading

Codecov Report

GitHK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odeimaiz Jun 1, 2021 • edited Loading

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

odeimaiz left a comment

Choose a reason for hiding this comment

ignapas commented Jun 1, 2021

sanderegg 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

pcrespov left a comment

Choose a reason for hiding this comment

ignapas commented Jun 3, 2021

odeimaiz commented Jun 3, 2021 • edited Loading

pcrespov left a comment • edited Loading

Choose a reason for hiding this comment

odeimaiz left a comment • edited Loading

Choose a reason for hiding this comment

ignapas commented Jun 10, 2021

odeimaiz commented Jun 11, 2021

pcrespov left a comment

Choose a reason for hiding this comment

ignapas commented May 27, 2021 •

edited

Loading

codecov bot commented May 27, 2021 •

edited

Loading

odeimaiz Jun 1, 2021 •

edited

Loading

odeimaiz commented Jun 3, 2021 •

edited

Loading

pcrespov left a comment •

edited

Loading

odeimaiz left a comment •

edited

Loading