Skip to content

Commit

Permalink
fix(test): resolve imports correctly in tests (#6478)
Browse files Browse the repository at this point in the history
**Problem:**
We have a major problem in tests where code like `import X from
'./file1.js'` is actually trying to resolve it to `test.js/file1.js` and
fails.
It happens due to `resolveTestFiles` calling:
```ts
path.normalize(`${importOrigin}/${normalizedName}`)
```
creating - for example - imports like `'test.js/app.js'` that weren't
found.
The function returned an error value of "Test error, the dumbResolveFn
did not know about this file: 'test.js/app.js'", but no one checked it
down the road, causing the valid paths generation to ignore its contents
completely.
The tests were asserting an incorrect snapshot, with partial, incorrect
valid paths, and that's the reason they passed.

**Fix:**
- Fix `resolveTestFiles` so it will be closer to our real code, using
`path.parse` to get the directory of the import origin
- Fix the tests snapshots to assert a correct expected result
- Log the error (throwing it seemed too obtrusive since some tests might
actually check for missing file functionality)

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode
  • Loading branch information
liady authored Oct 7, 2024
1 parent 98eda72 commit 1b19881
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 13 deletions.
26 changes: 18 additions & 8 deletions editor/src/components/canvas/ui-jsx-canvas-bugs.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export var storyboard = (
)
`,
{
'app.js': `
'/app.js': `
import React from 'react'
export default function App(props) {
return <div data-uid='app-outer-div'>
Expand All @@ -148,7 +148,7 @@ export default function App(props) {
id=\\"canvas-container\\"
data-testid=\\"canvas-container\\"
style=\\"position: absolute\\"
data-utopia-valid-paths=\\"sb sb/scene sb/scene/app\\"
data-utopia-valid-paths=\\"sb sb/scene sb/scene/app sb/scene/app:app-outer-div sb/scene/app:app-outer-div/inner-div\\"
data-utopia-root-element-path=\\"sb\\"
>
<div
Expand All @@ -175,8 +175,13 @@ export default function App(props) {
\\"
data-uid=\\"scene\\"
>
<div data-uid=\\"app-outer-div\\" data-path=\\"sb/scene/app\\">
<div data-uid=\\"inner-div\\">hello</div>
<div data-uid=\\"app-outer-div\\" data-path=\\"sb/scene/app:app-outer-div\\">
<div
data-uid=\\"inner-div\\"
data-path=\\"sb/scene/app:app-outer-div/inner-div\\"
>
hello
</div>
</div>
</div>
</div>
Expand Down Expand Up @@ -240,7 +245,7 @@ export var storyboard = (
)
`,
{
'app.js': `
'/app.js': `
import React from 'react'
export default function App(props) {
return <div data-uid='app-outer-div'>
Expand All @@ -256,7 +261,7 @@ export default function App(props) {
id=\\"canvas-container\\"
data-testid=\\"canvas-container\\"
style=\\"position: absolute\\"
data-utopia-valid-paths=\\"storyboard-entity storyboard-entity/scene-1-entity storyboard-entity/scene-1-entity/app-entity storyboard-entity/scene-2-entity storyboard-entity/scene-2-entity/same-file-app-entity storyboard-entity/scene-2-entity/same-file-app-entity:same-file-app-div\\"
data-utopia-valid-paths=\\"storyboard-entity storyboard-entity/scene-1-entity storyboard-entity/scene-1-entity/app-entity storyboard-entity/scene-1-entity/app-entity:app-outer-div storyboard-entity/scene-1-entity/app-entity:app-outer-div/inner-div storyboard-entity/scene-2-entity storyboard-entity/scene-2-entity/same-file-app-entity storyboard-entity/scene-2-entity/same-file-app-entity:same-file-app-div\\"
data-utopia-root-element-path=\\"storyboard-entity\\"
>
<div
Expand Down Expand Up @@ -286,9 +291,14 @@ export default function App(props) {
>
<div
data-uid=\\"app-outer-div\\"
data-path=\\"storyboard-entity/scene-1-entity/app-entity\\"
data-path=\\"storyboard-entity/scene-1-entity/app-entity:app-outer-div\\"
>
<div data-uid=\\"inner-div\\">hello</div>
<div
data-uid=\\"inner-div\\"
data-path=\\"storyboard-entity/scene-1-entity/app-entity:app-outer-div/inner-div\\"
>
hello
</div>
</div>
</div>
<div
Expand Down
2 changes: 1 addition & 1 deletion editor/src/components/canvas/ui-jsx-canvas-errors.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1465,7 +1465,7 @@ export var ${BakedInStoryboardVariableName} = (props) => {
}
`,
{
'app.js': `
'/app.js': `
import { throwError } from './card'
export function throwErrorFromCard() {
Expand Down
16 changes: 12 additions & 4 deletions editor/src/components/canvas/ui-jsx-canvas.test-utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,13 @@ export interface PartialCanvasProps {
}

export const dumbResolveFn = (filenames: Array<string>): CurriedResolveFn => {
return (_: ProjectContentTreeRoot) => (importOrigin: string, toImport: string) =>
resolveTestFiles(filenames, importOrigin, toImport)
return (_: ProjectContentTreeRoot) => (importOrigin: string, toImport: string) => {
const result = resolveTestFiles(filenames, importOrigin, toImport)
if (!isRight(result)) {
console.error(result.value)
}
return result
}
}

function resolveTestFiles(
Expand All @@ -72,7 +77,8 @@ function resolveTestFiles(
let normalizedName = normalizeName(importOrigin, toImport)
// Partly restoring what `normalizeName` strips away.
if (toImport.startsWith('.')) {
normalizedName = path.normalize(`${importOrigin}/${normalizedName}`)
const parsedOrigin = path.parse(importOrigin)
normalizedName = path.normalize(`${parsedOrigin.dir}/${normalizedName}`)
} else if (toImport.startsWith('/')) {
normalizedName = `/${normalizedName}`
}
Expand All @@ -92,7 +98,9 @@ function resolveTestFiles(
case UiFilePath:
return right(UiFilePath)
default:
return left(`Test error, the dumbResolveFn did not know about this file: ${toImport}`)
return left(
`Test error, the dumbResolveFn did not know about this file: ${toImport}, got ${normalizedName}`,
)
}
}

Expand Down

0 comments on commit 1b19881

Please sign in to comment.