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(app): render spec list, command log, iframe #18372

Merged
merged 41 commits into from
Oct 8, 2021

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Oct 6, 2021

This is the result of this spike ticket.

Here are my learnings, observations and recommendations going forward.

Details

  1. Do not import anything written to be bundled with webpack into Vite, no matter how trivial it seems. In the same way we do not import code from server into graphql or launchpad, we should respect this rule on the front-end too.
  2. Instead, the best way to consume webpack-based modules is to import them into @packages/runner-ct/src/main.tsx and add them to the window.UnifiedRunner object. That will the be bundled with webpack and injected onto window. You can then do things like window.UnifiedRunner.eventManager, window.UnifiedRunner.AutIframe, or even window.UnifiedRunner.React if you are feel edgy.
  3. So far, MobX seems to "just work". Ideally our source of truth will be GraphQL. Some state still needs to be duplicated in MobX, so just put that in packages/app/src/store. Ideally, we will minimize the amount of state managed in MobX.

What is done:

  • Proxy required modules from webpack -> Vite via window.UnifiedRunner namespace
  • Wrapper all the complexity around running a spec/initalizing AutIframe/resetting Reporter in a UnifiedRunnerAPI object with an executeSpec method
  • Added a lot of comments explaining it all, will update as we move forward
  • Show specs from GraphQL
  • You can execute CT specs in the new runner (using AutIframe workflow) by clicking a spec
  • It shows the reporter correctly
    • All the correct setup/teardown around driver/reporter etc is handled correctly

Todo (future PRs)

  • E2E specs (coming here)
  • Does not show the <reporter /> header (with buttons like re-run, pause, etc)
  • This is because we might be re-skinning this in Vue

Try it out

Here is a video showing how you can test it. Alternatively, see the updated app/README.md.

demo2.mov

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 6, 2021

Thanks for taking the time to open a PR!

@lmiller1990 lmiller1990 changed the base branch from develop to unified-desktop-gui October 6, 2021 04:49
@lmiller1990 lmiller1990 marked this pull request as ready for review October 6, 2021 08:14
@lmiller1990 lmiller1990 requested a review from a team as a code owner October 6, 2021 08:14
@lmiller1990 lmiller1990 requested review from flotwig, BlueWinds and JessicaSachs and removed request for a team, flotwig and BlueWinds October 6, 2021 08:14
Comment on lines +3 to +10
<!--
TODO(lachlan): put this back after doing proper cleanup when unmounting the runner page
keep-alive works fine for Vue but has some weird effects on the React based Reporter
For now it's way more simple to just unmount and remount the components when changing page.
-->
<!-- <keep-alive> -->
<component :is="Component" />
<!-- </keep-alive> -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... this is concerning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just never had the need to fully teardown everything, so that functionality didn't come "for free" with the existing code (think about it - we usually render the reporter, and never tear it down, in the existing runner). I think this should be an easy fix.

Can look into this in the mean time while you get your spec list PR ready.

@JessicaSachs
Copy link
Contributor

This LGTM. Let me merge my specs list in first

@lmiller1990 lmiller1990 changed the title feat(unified-app): render spec list, command log, iframe feat(app): render spec list, command log, iframe Oct 8, 2021
@lmiller1990 lmiller1990 merged commit a8b43ee into unified-desktop-gui Oct 8, 2021
tgriesser added a commit that referenced this pull request Oct 8, 2021
tgriesser added a commit that referenced this pull request Oct 8, 2021
tgriesser added a commit that referenced this pull request Oct 8, 2021
tgriesser added a commit that referenced this pull request Oct 10, 2021
* unified-desktop-gui: (40 commits)
  feat: index.html configurability and storybook support (#18242)
  fix: remove .json check from require_async, prevent child_process spawn (#18416)
  percy snapshot the tooltip visually, prevent it from being hidden
  fix: failing tests from #18372 (#18414)
  fix: `everyNthFrame` should only be applied for Chrome 89+ (#18392)
  feat(app): render spec list, command log, iframe (#18372)
  fix: drag and drop to be correct directory (#18400)
  refactor: Add GitDataSource, FileDataSource, toast for errors (#18385)
  docs: General updates to contributing guide (#18283)
  Add shorter --ct alias for --component
  Add --e2e and --component CLI options
  chore: Update Chrome (beta) to 95.0.4638.40 (#18389)
  chore: use circleci timings split for e2e tests (#18367)
  fix: fixed title (#18370)
  chore(deps): update dependency electron to v14 🌟 (#18384)
  chore(server): share client route (#18215)
  fix: Prevent Cypress from crashing when argument parsing "spec: {}" (#18312)
  chore: update husky dev dependency to v7 (#18345)
  feat: add defineConfig function to help type config (#18302)
  chore: Update Chrome (stable) to 94.0.4606.71 (#18324)
  ...
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.

None yet

2 participants