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(content-docs): suppress git error on multiple occurrences #6973

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

felipecrs
Copy link
Contributor

Motivation

#6937 (comment)

Have you read the Contributing Guidelines on pull requests?

Test Plan

I wrote tests.

Related PRs

#6937
cardano-foundation/developer-portal#557

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 23, 2022
@felipecrs
Copy link
Contributor Author

I created the temp folder and changed the temporary files to there (with a .gitignore) because I noticed that when the test fails, the temporary file may be left there.

@felipecrs felipecrs force-pushed the fix-logic branch 2 times, most recently from e44c80d to 4664856 Compare March 23, 2022 12:47
@felipecrs felipecrs changed the title fix(docs): suppress error on multiple occurrences fix(docs): suppress git error on multiple occurrences Mar 23, 2022
@@ -69,7 +69,12 @@ describe('getFileLastUpdate', () => {
const consoleMock = jest
.spyOn(console, 'warn')
.mockImplementation(() => {});
const tempFilePath = path.join(__dirname, '__fixtures__', '.temp');
const tempFilePath = path.join(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can reference the implementation in #6949 and use the system's temp folder instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it errors with:

not a git repository (or any of the parent directories): .git

In your case that was ok, since you needed to create a temp git repo dir, but I think here it's not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Creating a git repo isn't the most expensive thing we've done in our tests, so I think it's okay, compared to repeatedly writing files within our own repo. We used to do the same thing for the migration test, but that one was later refactored to use FS mocks instead.

In the near future, we would provide a unified tester interface so all git-related tests will be done within the same temp git repo, but for now, I'm okay with initializing another git repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Josh-Cena Right, understood. I need the

Function then. Any suggestion on how I can export it? I mean, I could simply "export" it and add:

// eslint-disable-next-line jest/no-export

To it. But, should I?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that seems a bit intractable. I'd rather not disable the rule, since you have to import it cross-module anyways. Can you lift this util to the jest folder and add a module name mapper like "@testing-utils/*": "<rootDir>/jest/utils"? We will eventually publish a separate package for that.

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 did it, just to show, but I can revert.

In the near future, we would provide a unified tester interface so all git-related tests will be done within the same temp git repo

Perhaps we should leave as is until then? If you prefer I can even revert my .gitignore addition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you lift this util to the jest folder and add a module name mapper like "@testing-utils/*": "/jest/utils"? We will eventually publish a separate package for that.

Nice. I will take a look.

@netlify
Copy link

netlify bot commented Mar 23, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 81d72af
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/623b1627d7fc060008789696
😎 Deploy Preview https://deploy-preview-6973--docusaurus-2.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.

@github-actions
Copy link

github-actions bot commented Mar 23, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 69
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-6973--docusaurus-2.netlify.app/

@netlify
Copy link

netlify bot commented Mar 23, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit c6fcdbd
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/623b2e69bc5f990008664ec4
😎 Deploy Preview https://deploy-preview-6973--docusaurus-2.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.

@Josh-Cena Josh-Cena changed the title fix(docs): suppress git error on multiple occurrences fix(content-docs): suppress git error on multiple occurrences Mar 23, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Thanks, exactly what I wanted 👍

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Mar 23, 2022
@slorber
Copy link
Collaborator

slorber commented Mar 23, 2022

Thanks, that looks nice ;)

@slorber slorber merged commit 4b3f568 into facebook:main Mar 23, 2022
@felipecrs felipecrs deleted the fix-logic branch March 23, 2022 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants