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

fix: discover CT community definitions in monorepos with hoisted dependencies #26066

Merged
merged 29 commits into from
Mar 14, 2023

Conversation

astone123
Copy link
Contributor

@astone123 astone123 commented Mar 9, 2023

Additional details

Right now when we detect CT community framework modules during the Launchpad setup, we only check the projectRoot node_modules/ and then return. This doesn't work if you have a monorepo that hoists dependencies to the repository root.

This PR adds logic to search from the project root to the repository root for community framework modules and return all of the ones that are found.

We use a variety of methods to determine whether or not we've found the repository root:

  • Looking for platform-specific configuration files (eg lerna.json, nx.json, etc)
  • Looking for a .git/ directory (meaning that Git has been configured at this level, indicating a repository root)
  • package.json having a workspace entry in it

This isn't a fool-proof way of determining the repository root but it should work for most projects. I basically copied what Vite does https://github.com/vitejs/vite/blob/0f6de4dcff783ec21fada47651d564cd5e2631b2/packages/vite/src/node/server/searchRoot.ts#L59

Steps to test

Use a monorepo, eg https://github.com/lmiller1990/ct-monorepo-bug

  • yarn install
  • notice @lmiller1990/cypress-ct-solid-js is in the root node_modules
  • cd packages/foo
  • open yarn in global mode yarn cypress:open
  • open ct-monorepo-bug/packages/foo in Cypress
  • select CT
  • verify that Solid shows up in the list as a community framework

How has the user experience changed?

PR Tasks

  • Have tests been added/updated? - I'm still working on this, will add another test for automatically testing monorepo support
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@astone123
Copy link
Contributor Author

I still need to add a test to automatically verify that we will find third party dependencies for monorepos, but I'm looking for some feedback in the mean time

@cypress
Copy link

cypress bot commented Mar 9, 2023

6 flaky tests on run #44729 ↗︎

0 4301 889 0 Flakiness 6

Details:

fix paths for windows
Project: cypress Commit: b172a098ed
Status: Passed Duration: 15:33 💡
Started: Mar 14, 2023 11:57 AM Ended: Mar 14, 2023 12:12 PM
Flakiness  commands/net_stubbing.cy.ts • 3 flaky tests • 5x-driver-webkit

View Output Video

Test Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse Output Video
... > with `times` > only uses each handler N times Output Video
... > stops waiting when an xhr request is canceled Output Video
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-webkit

View Output Video

Test Artifacts
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@@ -25,6 +26,46 @@ const thirdPartyDefinitionPrefixes = {
globalPrefix: 'cypress-ct-',
}

const ROOT_PATHS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit... should this be ROOT_FILES?

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 used the term paths because some of them are directories and some are files, for example .git is a directory.


packageJsonPaths = [...packageJsonPaths, ...newPackagePaths]

const isCurrentRepositoryRoot = await isRepositoryRoot(directory)
Copy link
Contributor

Choose a reason for hiding this comment

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

I can give this PR a test later, but definitely will be good to see some tests around this full functionality (whether it's Cy-in-Cy, or just an elaborate unit/integration test in this package).

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 added a unit test in this package, let me know what you think

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Tested it out, seems to be working as expected for the minimal reproduction.

Also, might be a good chance to make a little OSS module like find-project-root or something -- what do you think? Seems like something that'd be pretty useful.

Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

Testing this went fine @astone123, I see you have a few open comments from @lmiller1990 so feel free to tag me for re-review

@astone123 astone123 changed the title chore: discover CT community definitions in monorepos with hoisted dependencies fix: discover CT community definitions in monorepos with hoisted dependencies Mar 13, 2023
@@ -404,7 +404,15 @@ async function makeE2ETasks () {
}
},
async __internal_openProject ({ argv, projectName }: InternalOpenProjectArgs): Promise<ResetOptionsResult> {
if (!scaffoldedProjects.has(projectName)) {
let projectMatched = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to re-work this in order to call cy.openProject with a directory inside of a project like <projectName>/packages/foo to test monorepos

Copy link
Contributor

Choose a reason for hiding this comment

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

Shame we lose the auto completion :(

Let's do it for now, but I wonder if we can find a way to keep the completion 🤔

@@ -235,8 +235,10 @@ function openGlobalMode (options: OpenGlobalModeOptions = {}) {
})
}

function openProject (projectName: ProjectFixtureDir, argv: string[] = []) {
if (!fixtureDirs.includes(projectName)) {
type WithPrefix<T extends string> = `${T}${string}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, this allows us to open Cypress from within a project to support monorepo testing

@astone123 astone123 requested a review from lmiller1990 March 13, 2023 23:52
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Looks good, quick commit since I think this will break windows CI without path.join b172a09

@@ -404,7 +404,15 @@ async function makeE2ETasks () {
}
},
async __internal_openProject ({ argv, projectName }: InternalOpenProjectArgs): Promise<ResetOptionsResult> {
if (!scaffoldedProjects.has(projectName)) {
let projectMatched = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Shame we lose the auto completion :(

Let's do it for now, but I wonder if we can find a way to keep the completion 🤔


it('Scaffolds component testing for monorepos with hoisted dependencies', () => {
cy.scaffoldProject('ct-monorepo-unconfigured')
cy.openProject('ct-monorepo-unconfigured/packages/foo')
Copy link
Contributor

Choose a reason for hiding this comment

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

Random idea to keep the autocompletion on the project name:

cy.openProject('ct-monorepo-unconfigured', { package: 'packages/foo' })

@lmiller1990 lmiller1990 merged commit cacdb1d into develop Mar 14, 2023
@lmiller1990 lmiller1990 deleted the ct-third-party-monorepo-resolution branch March 14, 2023 12:13
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.

Cypress does not resolve CT Framework Definition in monorepo with hoisted node_modules
3 participants