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.

Sign up for GitHub

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

Jump to bottom

fix(content-docs): suppress git error on multiple occurrences #6973

Merged
merged 4 commits into from
Mar 23, 2022
Merged

fix(content-docs): suppress git error on multiple occurrences #6973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4664856
fix(docs): suppress error on multiple occurrences
felipecrs Mar 23, 2022
60e00c9
test: use common function to create git repo
felipecrs Mar 23, 2022
cf993fe
test: create @testing-utils
felipecrs Mar 23, 2022
c6fcdbd
test: remove unused temp dir
felipecrs Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/docusaurus-plugin-content-docs/src/__tests__/__fixtures__/temp/.gitignore
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 34 additions & 1 deletion packages/docusaurus-plugin-content-docs/src/__tests__/lastUpdate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

docusaurus/packages/docusaurus-utils/src/__tests__/gitUtils.test.ts

Line 16 in b456a64

function createTempRepo() {

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.

__dirname,
'__fixtures__',
'temp',
'file.md',
);
await fs.writeFile(tempFilePath, 'Lorem ipsum :)');
await expect(getFileLastUpdate(tempFilePath)).resolves.toBeNull();
expect(consoleMock).toHaveBeenCalledTimes(1);
Expand All @@ -79,6 +84,34 @@ describe('getFileLastUpdate', () => {
await fs.unlink(tempFilePath);
});

it('multiple files not tracked by git', async () => {
const consoleMock = jest
.spyOn(console, 'warn')
.mockImplementation(() => {});
const tempFilePath1 = path.join(
__dirname,
'__fixtures__',
'temp',
'file1.md',
);
const tempFilePath2 = path.join(
__dirname,
'__fixtures__',
'temp',
'file2.md',
);
await fs.writeFile(tempFilePath1, 'Lorem ipsum :)');
await fs.writeFile(tempFilePath2, 'Lorem ipsum :)');
await expect(getFileLastUpdate(tempFilePath1)).resolves.toBeNull();
await expect(getFileLastUpdate(tempFilePath2)).resolves.toBeNull();
expect(consoleMock).toHaveBeenCalledTimes(1);
expect(consoleMock).toHaveBeenLastCalledWith(
expect.stringMatching(/not tracked by git./),
);
await fs.unlink(tempFilePath1);
await fs.unlink(tempFilePath2);
});

it('git does not exist', async () => {
const mock = jest.spyOn(shell, 'which').mockImplementationOnce(() => null);
const consoleMock = jest
Expand Down
23 changes: 12 additions & 11 deletions packages/docusaurus-plugin-content-docs/src/lastUpdate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,18 @@ export async function getFileLastUpdate(
});
return {timestamp: result.timestamp, author: result.author};
} catch (err) {
if (err instanceof GitNotFoundError && !showedGitRequirementError) {
logger.warn('Sorry, the docs plugin last update options require Git.');
showedGitRequirementError = true;
} else if (
err instanceof FileNotTrackedError &&
!showedFileNotTrackedError
) {
logger.warn(
'Cannot infer the update date for some files, as they are not tracked by git.',
);
showedFileNotTrackedError = true;
if (err instanceof GitNotFoundError) {
if (!showedGitRequirementError) {
logger.warn('Sorry, the docs plugin last update options require Git.');
showedGitRequirementError = true;
}
} else if (err instanceof FileNotTrackedError) {
if (!showedFileNotTrackedError) {
logger.warn(
'Cannot infer the update date for some files, as they are not tracked by git.',
);
showedFileNotTrackedError = true;
}
} else {
logger.warn(err);
}
Expand Down