Skip to content

fix: don't duplicate .gitignore entries#122

Merged
mxschmitt merged 8 commits intomicrosoft:mainfrom
DerTimonius:patch-1
Jul 8, 2024
Merged

fix: don't duplicate .gitignore entries#122
mxschmitt merged 8 commits intomicrosoft:mainfrom
DerTimonius:patch-1

Conversation

@DerTimonius
Copy link
Copy Markdown
Contributor

Description

This PR fixes the duplication of existing .gitignore entries while running npm create playwright.

Although this only occurs when the command is run more than once, the added entries don't need to be duplicated.

Related to microsoft/playwright#31021

Comment thread src/generator.ts Outdated
Comment thread src/generator.ts Outdated
@DerTimonius
Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Copy link
Copy Markdown
Contributor

@hamirmahal hamirmahal left a comment

Choose a reason for hiding this comment

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

I was thinking it'd be kind of nice if the entries added by Playwright were separated by a heading, and they were in sorted order.

Something like this, for example

# Playwright
/blob-report/
/playwright-report/
/playwright/.cache/
/test-results/

Maybe these changes can go in this pull request, or I might be able to add them in a separate pull request, later.

Copy link
Copy Markdown
Contributor

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

I see that our only test-coverage so far is to check if the gitignore file exists. Maybe its time extend it that it contains the content we expect it to have.

Code pointer: https://github.com/microsoft/create-playwright/blob/3b986a1faf4f1d4b853ebbdb94c4c6e109e779b6/tests/integration.spec.ts#L31C41-L31C50

@DerTimonius
Copy link
Copy Markdown
Contributor Author

@mxschmitt good idea, I'm currently trying to create a test that runs the script twice, but the second run never finishes:

image

I was wondering, if there would be any other option to try to run the script twice?

@mxschmitt
Copy link
Copy Markdown
Contributor

@mxschmitt good idea, I'm currently trying to create a test that runs the script twice, but the second run never finishes:

image

I was wondering, if there would be any other option to try to run the script twice?

Feel free to push the changes into your branch, if it hangs, I'd be happy to take a look.

Comment thread tests/integration.spec.ts Outdated
Comment thread tests/integration.spec.ts Outdated
Comment thread tests/integration.spec.ts Outdated
Copy link
Copy Markdown
Contributor

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

Please re-base (since it has merge conflicts), apply comments and then we can merge it. The toMatch wasn't working before, hence it didn't fail on the test-results regex bug.

Comment thread src/generator.ts Outdated
Comment thread tests/integration.spec.ts Outdated
Comment thread tests/integration.spec.ts Outdated
Comment thread tests/integration.spec.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants