Create tests GH Actions workflow to run tests #3
Conversation
floogulinc
changed the title
|
Looks like the unit tests are failing due to issues with the tests themselves so I think this is ready. |
There was a problem hiding this comment.
@floogulinc – Thanks a ton for doing this. I was actually looking forward to actually trying to figure out GitHub Actions myself since we've always just relied on your work here. As a result I've asked a bunch of questions that I'd love to hear from you on.
Also, this is structured quite differently than the actions on, e.g., the project template from the Spring. Is there a particular reason for that?
You're correct that the tests are failing due a problem on our end. That's fixed in a branch and I'll cherry pick the one relevant commit into main after we merge in your GitHub Actions PR so that main will build successfully.
| @@ -15,7 +15,7 @@ exports.config = { | |||
| capabilities: { | |||
| browserName: 'chrome' | |||
| }, | |||
| directConnect: true, | |||
| directConnect: false, | |||
There was a problem hiding this comment.
I'm not sure why you changed this from the default. I can see why you might want to from what I've read on-line, but then I'm not sure why they have it set to true in the default config?
There was a problem hiding this comment.
We set this and moved to going through selenium in lab 3, 4, and the iteration template so we could use the element highlighting built into protractor.
There was a problem hiding this comment.
So this is necessary to make the highlighting work? Would it run (appreciably) faster if we didn't have that?
I'm not convinced that students use the highlighting much, especially once there are a lot of tests, although @kklamberty probably has differing opinions on that.
There was a problem hiding this comment.
If it's not needed then we could remove it and also possibly change back a lot of the custom stuff we did to allow for the highlighting but we would want to do a good amount of testing, especially on lab machines, to make sure it still works correctly with the more default setup.
phone-store/package.json
Outdated
| "e2e": "ng e2e" | ||
| "e2e": "ng e2e", | ||
| "pree2e": "node node_modules/protractor/bin/webdriver-manager -- update --gecko false", | ||
| "e2eci": "ng e2e --protractor-config=e2e/protractor-ci.conf.js" |
There was a problem hiding this comment.
Is this just here to provide a way call ng e2e with the right config file?
Was there a reason to add this here instead of just adding the flag to the e2e-tests job up above? (I'm not sure I care either way – just curious.)
There was a problem hiding this comment.
Yes this could be just copied into the GH Actions workflow instead of having it here. That might be better since it would be consistent with the unit tests. This is just how it was in the iteration template.
There was a problem hiding this comment.
I'm not sure I particularly care, but I did want to make sure I understood.
It does seem semi-odd to introduce the new e2eci task when we could just use the additional flag, though. Is there any setting outside of GitHub Actions where one might want to use ng e2eci? If so, then that might justify leaving it in. One could run ng e2eci on a droplet, for example, to make sure all the pieces work there – would that be potentially useful?
From the iteration template
|
A lot of the changes here come from what we did for lab 4 and the iteration template. I don't remember the exact reasons behind some of them and can't dig up the old conversations in Slack because we are so far over the 10k message limit. |
|
I found the PR where we made a lot of the e2e related changes in Lab 3: UMM-CSci-3601/3601-lab3_angular-javalin#386 |
|
In terms of the structure of having both tests in the same workflow file, this is somewhat a personal preference thing. I did it partly because of how GitHub displays these on mobile and in other places where it doesn't include the workflow name but only the job name: Screenshots
Also it seems like a good idea to have related jobs together like this, but this could change later when it's not just a client since the e2e tests need to also run stuff from the server, but we could have all three tests in the same workflow. The main thing to keep in mind is that the common thing in a workflow is the condition in which it is run, on push for example. |
This is a good argument for commenting things in these files. Otherwise we end up learning and relearning the same things over and over. 😜 I'm fine with having the tests in the same workflow file. It surprised me slightly after I'd been looking at the template from last semester, but I don't inherently care when things are this short. If this started to get really long that might be a reason to split it up, but we're definitely not there yet. Students might also find it less intimidating if it's all in one not-terribly-long file. One of your commits from Spring was to add some CSS for the highlighting. Is that still necessary? (It's not in this PR.) |
|
Yes we should definitely go through and comment some of these things. As for the highlighting CSS that was to change the default light blue to a more obvious yellow and force it to be above everything which was important especially for the Angular Material stuff but may still be helpful with the CSS in this project. We could certainly include it. |
|
I've got a meeting scheduled with @kklamberty Friday. Let me talk to her about the highlighting thing. How much would it simplify things if we just dropped highlighting altogether? A little? A lot? |
This very briefly mentions how the GitHub Actions were set up and the need to load the `webdriver-manager` binaries.
No description provided.