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

diff-npm-package: fix reports dir creation #3624

Merged
merged 1 commit into from Jun 2, 2022

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jun 1, 2022

#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.

graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
@netlify
Copy link

netlify bot commented Jun 1, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit ae54286
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6297ac980647870008f3a053
😎 Deploy Preview https://deploy-preview-3624--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 1, 2022

History:

  1. I noticed that there was a problem with pull requests in terms of creating the report file: See, for example: https://github.com/graphql/graphql-js/runs/6670599072?check_suite_focus=true#step:6:12.
  2. I surmised this was because of the recent PR diff-npm-package: move report into shared 'reports' folder #3611 changing the reports directory.
  3. I assumed that the problem was that the directory was not being created, so I submitted diff-npm-package: create reports folder #3623 to try to fix that.
  4. @IvanGoncharov noticed that there was something wrong with the check, and @saihaj submitted a fix for it, and diff-npm-package: create reports folder #3623 got merged.
  5. But, the truth is, that the report was not being created at all because no diff was found, so the check I added should never have been hit.
  6. I am unclear why the former PRs found a diff, but with the creation of diff-npm-package: create reports folder #3623, no diff was found. I find this very strange.
  7. Anyway, I am submitting what I hope is a better fix to diff-npm-package: create reports folder #3623, as the logic there unfortunately was still off, as the "reports" directory was being created with the same path later used as the report filename -- there needs to be two different paths, one for the creation of the folder, one for the creation of the file.
  8. I am not sure how to test this, as the report is not being created, and I am not sure how to actually force the report to be created.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 1, 2022

I am not sure why, but luckily #3622 still triggers a diff, and is still failing, so can be used to test this PR.

After rebasing, as expected, now mkdir is failing because we are calling it with the wrong path. Merging this PR should fix that, and so at least after we merge this, we can use that PR to see if the change works.

At that point, CI from #3622 should give us the diff, and we might be able to see why there is one!

@yaacovCR yaacovCR requested a review from a team June 2, 2022 10:19
@yaacovCR yaacovCR changed the title diff-npm-package: polish reports dir creation diff-npm-package: fix reports dir creation Jun 2, 2022
@IvanGoncharov
Copy link
Member

Thanks for the detailed, investigation 👍
Your description gives a lot of contexts.

I am not sure how to actually force the report to be created.

Any change that affects the content of the NPM file (add stuff to package.json, LICENSE, README, or any files under 'src/').

@IvanGoncharov
Copy link
Member

Context: I need this script to see what changed inside the NPM package.
It's relevant on PRs that update tsconfig.json, resources/build-npm.js, etc.
It was super useful during TS migration.
Before that, I was doing the same by cloning PRs and generating before/after builds and running diff on it.
For example, it would be relevant in upcoming CJS/MJS discussions.

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.

None yet

2 participants