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

Update build-and-test to prevent merging fdescribe and fit #2416

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Apr 17, 2024

It's very easy to accidentally include fdescribe and fit when opening a PR. This has happened multiple times, most recently in #2373 (which actually has an approved review, and I was about to merge).

This PR updates the build workflow to grep for fdescribe( and fit( across all .spec.ts files, and fails the build if they are present.

It's also possible to accidentally include xdescribe and xit, but sometimes those will be intentional, and generally it's only a few tests that would be skipped over.

This PR changes a describe to fdescribe to demonstrate that the change works. Therefore, it deliberately fails the build. After being reviewed, I will revert that one change so it can be merged.


This change is Reviewable

@pmachapman pmachapman self-assigned this Apr 17, 2024
@pmachapman pmachapman self-requested a review April 17, 2024 18:45
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)


.github/workflows/build-and-test.yml line 87 at r1 (raw file):

        # grep returns code 123 when no matches are found. The operator ! negates the exit code.
        run: |
          ! git ls-files | grep "\\.spec\\.ts" | xargs grep -P "(fdescribe|fit)\("

This regex might be a bit too broad - it will pick up fdescribe/fit in comments, or method names like unfit().

Should we limit it to fdescribe/fit preceded by whitespace, from the start of the line?

git ls-files | grep "\\.spec\\.ts" | xargs grep -P "^\s*(fdescribe|fit)\("

Code quote:

git ls-files | grep "\\.spec\\.ts" | xargs grep -P "(fdescribe|fit)\("

@Nateowami Nateowami force-pushed the ci/prevent-fdescribe-and-fit branch from 4a101a2 to 756a8d4 Compare April 17, 2024 18:55
@Nateowami Nateowami requested a review from pmachapman April 17, 2024 18:55
Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @pmachapman)


.github/workflows/build-and-test.yml line 87 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This regex might be a bit too broad - it will pick up fdescribe/fit in comments, or method names like unfit().

Should we limit it to fdescribe/fit preceded by whitespace, from the start of the line?

git ls-files | grep "\\.spec\\.ts" | xargs grep -P "^\s*(fdescribe|fit)\("

Good improvement. Done.

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

@Nateowami Nateowami force-pushed the ci/prevent-fdescribe-and-fit branch from 756a8d4 to 852acba Compare April 17, 2024 19:07
@Nateowami Nateowami requested a review from pmachapman April 17, 2024 19:12
@Nateowami Nateowami merged commit 475bbdf into master Apr 17, 2024
13 of 14 checks passed
@Nateowami Nateowami deleted the ci/prevent-fdescribe-and-fit branch April 17, 2024 21:56
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