Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(launchpad): detect if runner is set up and change UI accordingly #17934

Merged

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Aug 30, 2021

https://cypress-io.atlassian.net/browse/UNIFY-292

Requirements of ticket:

  • changes the UI to show "LAUNCH" if the testing type (CT or E2E) is configured
  • otherwise it says "Click here to configure... "
  • clicking "LAUNCH" launches the CT runner
    • At this point in time, a testing type is considered "configured" if you have at least one property inside of the component or e2e keys inside of cypress.json. This will change when feat: make cypress.config.js available #17000 is merged (where we will use the devServer or setupNode key sin plugins to figure out if CT/E2E has been initialized).

Some other things were added to get launchpad to be a bit more functional in general:

  • added some code to cover some edge cases. Namely, sometimes the electron app can start before graphql has initialized the project (by initialize, I mean resolve the configuration and initialize plugins). We poll until the graphql server has started, and the current project has been initalized (config, plugins)
    • in the future, we probably won't want to initialize plugins until the user actually selects a runner. At this point, everything is very tightly coupled by the open_project singleton, and will take a bit of heavy lifting to uncouple. This will be okay until we have something a bit better.

Using launchpad as an example, since we have configured CT, it shows "launch" for the CT card. E2E has not been configured, so it shows "Click here to configure ...". These are not the final labels, just placeholders.

image

@lmiller1990 lmiller1990 requested a review from a team as a code owner August 30, 2021 06:58
@lmiller1990 lmiller1990 requested review from flotwig, chrisbreiding, tgriesser, JessicaSachs and ZachJW34 and removed request for a team, flotwig and chrisbreiding August 30, 2021 06:58
circle.yml Outdated
@@ -386,7 +386,7 @@ commands:
type: string
steps:
- restore_cached_workspace
- run: yarn workspace @packages/launchpad cypress:run --browser <<parameters.browser>>
- run: yarn workspace @packages/launchpad build && yarn workspace @packages/launchpad cypress:run --browser <<parameters.browser>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out build is not necessary here, I was wrong about the root cause of the flake.

Copy link
Member

@tgriesser tgriesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments - also have been wondering if there's value to the App type, or if we should just move those fields to the top level Query

"""
Whether the user has configured e2e testing or not, based on the existance of a 'component' key in their cypress.json
"""
hasSetupE2ETesting: Boolean!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, love the descriptive graphql fields 👍

`

gql`
fragment TestingTypeCardsWizard on Wizard {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason these two couldn't be consolidated into:

fragment TestingTypeCards on Query {
  app {
    activeProject {
      hasSetupComponentTesting
      hasSetupE2ETesting
    }
  }
  wizard {
    testingTypes {
      id
      title
      description
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not get mountFragment to work with Fragments on the top level Query. I wonder if I'm missing something.

const ct = computed(() => {
const type = props.gql?.wizard?.testingTypes?.find(x => x.id === 'component')
if (!type) {
throw Error('Did not get testingType from API')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this possible and if so, should we make the fields non-null so it's not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}

interval = window.setInterval(poll, 200)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I wonder if there's a better way we should be handling things on the server so this race condition isn't a frontend concern.

Copy link
Contributor Author

@lmiller1990 lmiller1990 Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say so. If some activities need to occur before the UI is rendered, I suppose something like

async function start (openElectron) {
  // do stuff
  await initializeThings()

  openElectron()
}

I'd say once we untangle the mess that is open_project, this problem will natural disappear.

@lmiller1990 lmiller1990 merged commit 992d223 into unified-desktop-gui Aug 31, 2021
@lmiller1990
Copy link
Contributor Author

@tgriesser made most of the recommended changes and we green, so going to merge. We should explore:

  1. feat(launchpad): detect if runner is set up and change UI accordingly #17934 (comment) - mountFragment does not seem to type correctly with top-level Query. I tried fixing it but need to spend a lot more time to really get how the mount codegen works.
  2. Better way to spin everything up and solve a potential race condition - as you noted this isn't ideal.

tgriesser added a commit that referenced this pull request Sep 1, 2021
…p-gui-gql-stitching

* unified-desktop-gui:
  fix: rename testApolloClient -> testUrqlClient, add apollo.config.js (#17948)
  feat(launchpad): detect if runner is set up and change UI accordingly (#17934)
  rebuild yarn lock
  chore: remove skipLibCheck, re-add vue-tsc, fix types (#17924)
  fix: ensure @headlessui/vue is bundled
  chore(graphql): make tsconfig more strict (#17909)
  run vite during watch
  fix: source maps while developing electron
  rebuild schema w/o JS files
  make constant more generic
  remove hanging tests
  allow larger artifacts
  comment out flaky test
  fix typing errors
  make graphql tsconfig more minimal
  chore: use `import type` syntax (#17864)
  remove browser-specific `beforeinput` checks (send in firefox) (#17820)
  fix(types): Update title method options type (#17781)
  fix(deps): update dependency @cypress/request to v2.88.6 🌟 (#17813)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants