test: UNIFY-1310 add tests to runner #20873

rachelruderman · 2022-03-31T18:18:59Z

Closes UNIFY-1310

User facing changelog

n/a

Additional details

Adds test to verify that text changes when the Request Access button is clicked
Adds test to verify projectId is inserted into config file when a new project is created
Removes try/catch that was silencing an error message

How has the user experience changed?

n/a

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?
[na] Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-03-31T18:19:01Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

CLAassistant · 2022-03-31T18:19:08Z

All committers have signed the CLA.

cypress · 2022-04-01T17:58:22Z

Test summary

17812 • 0 • 217 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`1da8e0e`
Started	Apr 6, 2022 7:10 PM
Ended	Apr 6, 2022 7:22 PM
Duration	12:18 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

…ress into unify-1310-run-management

rachelruderman · 2022-04-04T06:03:03Z

packages/graphql/src/schemaTypes/objectTypes/gql-Mutation.ts

-        try {
-          await ctx.actions.project.setProjectIdInConfigFile(args.projectId)
-        } catch {
-          // ignore error as not useful for end user to see
-        }
+        await ctx.actions.project.setProjectIdInConfigFile(args.projectId)


🤣 😭 this ignored error was painful for me to track down. I think there's value in having it:

Even if it's not immediately useful for the end user to see, it becomes very useful if/when the end user reports a bug. Without this error, the "Create project" button just fails silently.

hmmm is a way to catch this and provide an useful error message to the end-user so the error isn't hidden while giving the user information on where to find help or how to solve it?

We end up showing two different errors different places, one of which is the launchpad, which does not seem ideal. @tgriesser is this expected? I only found this error when I went looking for the little blue toast to restart Cypress.

I made https://cypress-io.atlassian.net/browse/UNIFY-1530 to track the error question, I do think it's better not to catch here so that this error goes somewhere, even if the launchpad is not the right place.

Yeah, we probably need to sync on this and figure out how to make this actionable as part of the response of the mutation, rather than having this be unhandled and displayed like this.

Any error in a mutation should be caught/handled, otherwise it shows up as the "An unexpected internal error occurred" as above, which we don't really want. Maybe we want to log the error out to the console or something?

I think the way the error case handled is a bit hard to follow in this flow and could be improved, it's almost like we want the return type of the mutation to be a union of Query | ErrorWrapper

rachelruderman · 2022-04-04T06:03:33Z

packages/app/src/runs/modals/SelectCloudProjectModal.vue

-  if (newProject.value && pickedOrganization.value) {
+  const isNewProject = Boolean(newProject.value && pickedOrganization.value)
+
+  if (isNewProject) {


Just threw in an intermediate variable for clarity

emilyrohrbough · 2022-04-04T15:53:53Z

packages/app/cypress/e2e/runs.cy.ts

+      cy.withCtx(async (ctx) => {
+        const config = await ctx.project.getConfig()
+
+        expect(config.projectId).to.not.equal('newProjectId')


would this value be undefined?

At this point it's null. I checked to.not.equal('newProjectId') instead of to.equal(null) so that if at some poit in the future a projectId is added to the scaffolded project, this test won't break 😃 happy to change if you prefer

so that if at some point in the future a projectId is added to the scaffolded project

I'm not as familiar with the dashboard space, but if a new scaffolded project had a projectId, are users able to create a new project & override it from the UI?

if a new scaffolded project had a projectId, are users able to create a new project & override it from the UI

I believe so

emilyrohrbough · 2022-04-04T15:55:21Z

packages/app/cypress/e2e/runs.cy.ts

@@ -171,6 +171,44 @@ describe('App: Runs', { viewportWidth: 1200 }, () => {
    })
  })

+  context('Runs - Create Project', () => {


Not specific to your changes, but with name of this test file I wasn't expecting these tests to be specific to the dashboard.

I see that some of the specs in this directory have the cypress-in-cypress prefix, i.e. cypress-in-cypress-run-mode.cy.ts. Should I update to something like cypress-in-cypress-open-mode-runs.cy.ts or more simply dashboard-runs.cy.ts?

We should probably retire the "cypress-in-cypress" convention, it refers to solving a specific problem, which was getting the spec runner to work in Cypress tests.

I do think "runs" is an OK name, since the the app contexts, runs and the "runs page" are all about test runs recorded to the dashboard. But runs_page is probably clearer file name if we want one, to be more clear that this is checking the display of runs info from the cloud, not anything at all about the behavior of the runner.

I even got confused writing that so we probably do need a better name.

Not that there are getting to be more files in here, it might be a good time to step back and organize this folder with more of pattern... some files test pages, some test multi-page features like the top-nav, and the names of the cypress-in-cypress tests are quite unclear.

I do think "runs" is an OK name, since the the app contexts, runs and the "runs page" are all about test runs recorded to the dashboard.

Given I haven't worked in the dashboard, I alway associate runs to a spec run or test run within a spec. Def an overloaded term.!

I do agree cypress-in-cypress solved a problem that doesn't necessarily need called out.

emilyrohrbough · 2022-04-05T20:20:46Z

packages/app/cypress/e2e/runs.cy.ts

+
+      cy.visitApp('/runs')
+      cy.findByText(defaultMessages.runs.errors.unauthorized.button).click()
+      cy.findByText(defaultMessages.runs.errors.unauthorizedRequested.button).should('exist')


Just curious, this the context of this test, but is the following describe block of 'Runs - Unauthorized Project Requested' testing something different compared to the current describe block of Runs - Unauthorized Project? Wondering if we just need to enhance one of these? If not, what am I missing?

AFAIK in 'Runs - Unauthorized Project Requested', we're validating the correct UI appears for users who have previously requested access, whereas in Runs - Unauthorized Project, we're testing the request access flow itself

marktnoonan

Provokes a lot of thoughts about the code around the PR, but the changes look good and it adds the test specified in the ticket. Happy to approve again if you make more changes.

marktnoonan · 2022-04-05T20:56:32Z

packages/graphql/src/schemaTypes/objectTypes/gql-Mutation.ts

-        try {
-          await ctx.actions.project.setProjectIdInConfigFile(args.projectId)
-        } catch {
-          // ignore error as not useful for end user to see
-        }
+        await ctx.actions.project.setProjectIdInConfigFile(args.projectId)


I made https://cypress-io.atlassian.net/browse/UNIFY-1530 to track the error question, I do think it's better not to catch here so that this error goes somewhere, even if the launchpad is not the right place.

davidmunechika and others added 3 commits March 17, 2022 12:29

add test for create project

673d15f

merge

789fb72

Clean up test names

5ba1dd8

merge latest

d1e4fcb

rachelruderman changed the title ~~add test for create project~~ test: UNIFY-1310 validate text after clicking request access button Apr 1, 2022

rachelruderman added 2 commits April 1, 2022 12:40

add test for button click

3106447

Merge branch '10.0-release' into unify-1310-run-management

4508410

rachelruderman marked this pull request as ready for review April 1, 2022 19:43

rachelruderman marked this pull request as draft April 1, 2022 19:44

rachelruderman added 3 commits April 1, 2022 12:45

use else if

816dfbd

clean up diffs

6b6d086

gerge branch 'unify-1310-run-management' of github.com:cypress-io/cyp…

a57beb6

…ress into unify-1310-run-management

rachelruderman changed the title ~~test: UNIFY-1310 validate text after clicking request access button~~ test: UNIFY-1310 add tests to runner Apr 1, 2022

add config file test

0fa6095

rachelruderman marked this pull request as ready for review April 4, 2022 05:58

rachelruderman commented Apr 4, 2022

View reviewed changes

rachelruderman added 3 commits April 3, 2022 23:06

resolve check-ts error

7d7495b

try setting projectId inside gql intercept

de425ff

do not use variable for newProjectId

f50b498

tbiethman requested review from emilyrohrbough and marktnoonan April 4, 2022 15:41

emilyrohrbough reviewed Apr 4, 2022

View reviewed changes

rachelruderman added 3 commits April 4, 2022 09:34

Merge branch '10.0-release' into unify-1310-run-management

91853eb

dummy push

9914d2a

dummy push

25fe7a6

dummy push

68d562d

flotwig requested a review from emilyrohrbough April 5, 2022 15:35

emilyrohrbough reviewed Apr 5, 2022

View reviewed changes

marktnoonan approved these changes Apr 5, 2022

View reviewed changes

emilyrohrbough approved these changes Apr 6, 2022

View reviewed changes

rachelruderman added 3 commits April 6, 2022 10:01

rename test context

f6509ed

Merge branch '10.0-release' into unify-1310-run-management

b404778

Merge branch '10.0-release' into unify-1310-run-management

1da8e0e

rachelruderman merged commit 1ef7ffb into 10.0-release Apr 6, 2022

rachelruderman deleted the unify-1310-run-management branch April 6, 2022 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: UNIFY-1310 add tests to runner #20873

test: UNIFY-1310 add tests to runner #20873

rachelruderman commented Mar 31, 2022 •

edited

Loading

cypress-bot bot commented Mar 31, 2022

CLAassistant commented Mar 31, 2022 •

edited

Loading

cypress bot commented Apr 1, 2022 •

edited

Loading

rachelruderman Apr 4, 2022

emilyrohrbough Apr 5, 2022

marktnoonan Apr 5, 2022

marktnoonan Apr 5, 2022

tgriesser Apr 6, 2022

rachelruderman Apr 4, 2022

emilyrohrbough Apr 4, 2022

rachelruderman Apr 4, 2022

emilyrohrbough Apr 5, 2022

rachelruderman Apr 6, 2022 •

edited

Loading

emilyrohrbough Apr 4, 2022

rachelruderman Apr 4, 2022 •

edited

Loading

marktnoonan Apr 5, 2022 •

edited

Loading

emilyrohrbough Apr 5, 2022 •

edited

Loading

emilyrohrbough Apr 5, 2022 •

edited

Loading

rachelruderman Apr 6, 2022

marktnoonan left a comment

marktnoonan Apr 5, 2022

test: UNIFY-1310 add tests to runner #20873

test: UNIFY-1310 add tests to runner #20873

Conversation

rachelruderman commented Mar 31, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Mar 31, 2022

CLAassistant commented Mar 31, 2022 • edited Loading

cypress bot commented Apr 1, 2022 • edited Loading

Test summary

Run details

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

rachelruderman Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachelruderman Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

marktnoonan Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marktnoonan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachelruderman commented Mar 31, 2022 •

edited

Loading

CLAassistant commented Mar 31, 2022 •

edited

Loading

cypress bot commented Apr 1, 2022 •

edited

Loading

rachelruderman Apr 6, 2022 •

edited

Loading

rachelruderman Apr 4, 2022 •

edited

Loading

marktnoonan Apr 5, 2022 •

edited

Loading

emilyrohrbough Apr 5, 2022 •

edited

Loading

emilyrohrbough Apr 5, 2022 •

edited

Loading