tests: bring fixtures in line with templates

[MichaelDeBoey]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: a9a72ac

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[brophdawg11]

To make this simpler in the future, I added a scripts/copy-templates-to-fixtures.sh script to do this and ran it - which brought along a few other small changes in 13aafdd. Any concerns with that approach?

[MichaelDeBoey]

@brophdawg11 I think we should keep this manual as we'll always have subtle small changes like having the 0.0.0-local-version or actual latest version, or like we now have the problem where having links in the Node template fixture makes it crash, ...

[brophdawg11]

Something in a fixture is causing a crash but it's not failing any tests? That feels like it should cause a test failure...?

My personal stance is that large-scale changes like this that are done manually worry me since it's easy to mess it up manually and hard to detect in a PR since the diff is so large (a bad combination). I think some form of automated copy or some form of being able to read directly from templates in tests would make it much easier to handle these and get them merged in the future.

[MichaelDeBoey]

Something in a fixture is causing a crash but it's not failing any tests?

@brophdawg11 It's not because I commented it out (for now)
See integration/helpers/node-template/app/root.tsx's links

[MichaelDeBoey]

@brophdawg11 It seems like we had a bug that I just discovered due to these updates
@markdalgleish fixed it in #7233

[brophdawg11]

Am I understanding this correctly?

• integration/helpers/node-template/app/root.tsx links used to have to be commented out due to the bug fixed by fix(dev): remove unused css-bundle path resolution #7233. If not commented out it would have crashed/caused a test failure?
• Now that fix(dev): remove unused css-bundle path resolution #7233 is fixed - it no longer causes a crash, and that's why it no longer needs to be commented out and the tests are all passing?

subtle small changes like having the 0.0.0-local-version or actual latest version

Is this actually a problem? The unit tests don't care about the version since they're running off local repo code anyway.

If I run the copy script, I see 3 main things that get updated:

• meta functions are aligned - this is 👍
• @remix-run/* deps go to * versions - I do not thinkg this has any impact on the unit tests, but please correct me if I'm wrong
• Deno's fixture handler creation moves from mode: build.mode to mode: Deno.env.get("NODE_ENV") - which also seems 👍 to me and doesn't cause failures

I'm just not seeing any reason we want to only bring these 95% in line instead of 100% in-line with templates if the actual templates being used don't cause any issues?

And beyond that - I'm half wondering why our uint etsts don't just run against the templates and why we can';t delete the fixtures entirely - but that's a larger question that's beyond the scope here I think.

[MichaelDeBoey]

• integration/helpers/node-template/app/root.tsx links used to have to be commented out due to the bug fixed by fix(dev): remove unused css-bundle path resolution #7233. If not commented out it would have crashed/caused a test failure?
• Now that fix(dev): remove unused css-bundle path resolution #7233 is fixed - it no longer causes a crash, and that's why it no longer needs to be commented out and the tests are all passing?

@brophdawg11 Correct

Is this actually a problem? The unit tests don't care about the version since they're running off local repo code anyway.

I presume it is, I wouldn't know why it's otherwise set to 0.0.0-local-version tbh 🤷‍♂️

• @remix-run/* deps go to * versions - I do not thinkg this has any impact on the unit tests, but please correct me if I'm wrong

Same reasoning as the 0.0.0-local-version versions here

• Deno's fixture handler creation moves from mode: build.mode to mode: Deno.env.get("NODE_ENV") - which also seems 👍 to me and doesn't cause failures

Seems like I missed updating that one in the Deno template in #7089 🙈

I'm just not seeing any reason we want to only bring these 95% in line instead of 100% in-line with templates if the actual templates being used don't cause any issues?

I updated the script to also copy the templates into the integration/helpers folder
Things that pop up extra about the integration fixtures that aren't in templates:

• scripts that aren't linked to node_modules properly
• eslint is removed
• ...global.INJECTED_FIXTURE_REMIX_CONFIG, is removed from remix.config.js
• node fixture has @vanilla-extract/css as extra dependency
  Should this be handled in the tests itself? cc/ @markdalgleish

And beyond that - I'm half wondering why our uint etsts don't just run against the templates and why we can';t delete the fixtures entirely - but that's a larger question that's beyond the scope here I think.

That's indeed beyond the scope of this ticket
But using the template folders for all fixture testing (in both @remix-run/dev & integration tests) is definitely something we can look at in a next PR I think

[markdalgleish]

node fixture has @vanilla-extract/css as extra dependency
Should this be handled in the tests itself?

@MichaelDeBoey I guess we could manually define a custom package.json in the Vanilla Extract test?

[MichaelDeBoey]

@brophdawg11 @markdalgleish I added package.json to the tests that needed @vanilla-extract/css and removed it from the fixture

The other problems I raised are still not fixed though 🤷‍♂️

[brophdawg11]

@MichaelDeBoey yeah I actually wouldn't try to tackle integration test fixtures in this PR or with the script since those are actual running apps so commands/package.json stuff is important, whereas in unit tests we're not installing dependencies so the package.json stuff is effectively irrelevant. I confirmed with @pcattori that the local version numbers are not important for unit test fixtures.

Can we put this back to just a script to update unit tests fixtures and get that merged, and tackle any updates to integration test fixtures in a separate PR?

[MichaelDeBoey]

Can we put this back to just a script to update unit tests fixtures and get that merged, and tackle any updates to integration test fixtures in a separate PR?

@brophdawg11 This PR was always about both updating integration + unit test fixtures, the script you made was only about unit test fixtures

I reverted the changes I made to your script, so it's only updating the unit test fixtures again

[brophdawg11]

Just one question otherwise LGTM

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-c28c3cc-20230827 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 2.0.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!