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

Fixed and improved tests for non-npm usage #29

Merged

Conversation

Maxim-Durand
Copy link
Contributor

Following changes:

  1. File path used in fake commits for tests were from the wrong directory, fixed that.

  2. currentWorkspace definition wasn't generic and would fail for other project file structure, see the substring(11) in

    currentWorkspace = workspaceDeps[Object.keys(workspaceDeps)[0] as any].resolved.substring(11)

  3. Also removed:

  // auto adds the current path to the file paths reported from git, so we need to undo this
  const fixedFiles = commit.files.map((file) => path.relative(currentDir, file))

as I couldn't see this behaviour when testing, instead I think the reason for this codepath was the mistake in 1.

  1. Duplicated the test batteries for non-npm usage.

…pm and non-npm. Improved tests and added test suite for non-npm usage.
@Maxim-Durand
Copy link
Contributor Author

@restfulhead After some testing I realised the latest release of this plugin is broken as filtering doesn't seem to work for non-npm usage.

This PR should hopefully fixes the issue but it's hard to know for sure as I need the new version of the code to be released before importing it in my projects (because local auto plugins are broken see intuit/auto#2019).

@restfulhead
Copy link
Owner

restfulhead commented Apr 5, 2024

@Maxim-Durand Thanks for testing and providing a fix. I will try to test this for the NPM use case later.

// auto adds the current path to the file paths reported from git, so we need to undo this
as I couldn't see this behaviour when testing, instead I think the reason for this codepath was the mistake in 1.

Update: Auto does a resolve on the paths reported by Git, but I can see how changing relativeWorkspacePath removes the need for the workaround. And good catch regarding the wrong test data. Going to merge this now.

@restfulhead restfulhead merged commit e511d78 into restfulhead:main Apr 6, 2024
1 check passed
@restfulhead
Copy link
Owner

Released to NPM as v0.4.0

@Maxim-Durand
Copy link
Contributor Author

Thanks for the quick release ! 🙏

After some testing I realised my change 3. was incorrect and there is indeed a bug in auto making it append the execution dir as a prefix to the commit files.

I found the reason why and fixed the bug in intuit/auto#2456

Will report back when it's merged to see if this plugin does work as expected on non-npm usage.

@restfulhead
Copy link
Owner

I found the reason why and fixed the bug in intuit/auto#2456

@Maxim-Durand Given that this is probably a breaking change and that it'll take time to merge, I think we should revert the path changes and cut another release until it's fixed. Would you agree?

@Maxim-Durand
Copy link
Contributor Author

I found the reason why and fixed the bug in intuit/auto#2456

@Maxim-Durand Given that this is probably a breaking change and that it'll take time to merge, I think we should revert the path changes and cut another release until it's fixed. Would you agree?

Sorry I didn't see your message sooner.

Yes I also agree the change 3. should be reverted for now.

Honestly, I'm not even sure auto is still managed anymore since it's been more than 2 months without any new reviews (not a single PR got merged).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants