Skip to content

Restore ability for packages to focus specs… #1265

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

Merged
merged 2 commits into from
Apr 14, 2025
Merged

Conversation

savetheclocktower
Copy link
Contributor

…on both Jasmine 1 and Jasmine 2 test runners.

Also restore behavior of failing immediately in CI if a test is accidentally left focused.

These were some loose ends left over from #990. Focusing specs (changing it to fit or describe to fdescribe) was already working within Pulsar's own test suite for Jasmine 2 tests, but not on package specs running Jasmine 2, nor on package specs running Jasmine 1.

I don't like it when my head hurts, so I've not delved too deeply into determining how the core suite was able to use fit (despite the fact that I couldn't figure out how or where it was defined) but not a package. I just know that the jasmine-focused and jasmine2-focused packages solve this for Jasmine 1 and 2, respectively.

The Jasmine 2 runner also needed a way to fail the tests in CI if any test was accidentally left focused. I found this out the hard way when that very thing happened to me in #1249.

@DeeDeeG
Copy link
Member

DeeDeeG commented Apr 5, 2025

Looks basically reasonable, I think? Large linter diff is kinda distracting on a PR like this, I am highly tempted to offer to split those off to a separate commit or yeet those parts of the diff if you don't mind. I could force push the branch even?

EDIT: If I am reading correctly, spec/runners/jasmine1-test-runner.js may be only whitespace changes, it should be fairly easy to exclude that file from the commit. I can easily offer to do that much at least.

EDIT 2: Of course, there's the require('jasmine-focused');. Anyhow, I can offer to split those changes to their own commit.

@savetheclocktower
Copy link
Contributor Author

Looks basically reasonable, I think? Large linter diff is kinda distracting on a PR like this, I am highly tempted to offer to split those off to a separate commit or yeet those parts of the diff if you don't mind. I could force push the branch even?

EDIT: If I am reading correctly, spec/runners/jasmine1-test-runner.js may be only whitespace changes, it should be fairly easy to exclude that file from the commit. I can easily offer to do that much at least.

EDIT 2: Of course, there's the require('jasmine-focused');. Anyhow, I can offer to split those changes to their own commit.

Feel free if you think it's worthwhile. (I swear I'm not doing it on purpose; it's just how linter-eslint works with our root .eslintrc — saving means autofixing. Often I don't even realize it's happened.)

…on both Jasmine 1 and Jasmine 2 test runners.

Also restore behavior of failing immediately in CI if a test is accidentally left focused.
@DeeDeeG DeeDeeG force-pushed the testing-revisited branch from e2163d1 to 5850d7b Compare April 8, 2025 07:29
Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, looks good to me, and it's good to not allow a lot of tests to be subtly (and likely accidentally) disabled by focusing just one or two tests and failing to un-focus them!

@DeeDeeG DeeDeeG merged commit aa0b123 into master Apr 14, 2025
101 of 103 checks passed
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.

2 participants