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

Add consistent-spacing-between-blocks rule #340

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Conversation

bjornua
Copy link
Contributor

@bjornua bjornua commented Jan 5, 2024

This PR introduces a new rule mocha/consistent-spacing-between-blocks, which enforces consistent line breaks between Mocha testing blocks (before, after, describe, it, beforeEach, afterEach) within describe blocks.

The following patterns are considered errors:

describe("MyComponent", function () {
    beforeEach(function () {
        // setup code
    });
    it("should behave correctly", function () {
        // test code
    });
});

These patterns would not be considered errors:

describe("MyComponent", function () {
    beforeEach(function () {
        // setup code
    });

    it("should behave correctly", function () {
        // test code
    });

    afterEach(function () {
        // teardown code
    });
});

The motivation behind this rule is to improve the readability of test code, making it easier to distinguish different sections. To me, this convention seems very common. It's also used in all the examples in https://mochajs.org/. Happy to hear your thoughts

@bjornua bjornua marked this pull request as ready for review January 5, 2024 10:27
@bjornua bjornua changed the title Add mocha/consistent-spacing-between-blocks rule Add consistent-spacing-between-blocks rule Jan 5, 2024
@lo1tuma
Copy link
Owner

lo1tuma commented Jan 5, 2024

Seems like this is related to #145 and prior work has been done in #185 but it got never finished. Although #145 mentions a few other styling preferences like never having padding between test functions or always having them. Ideally we can support all of them in one rule and having an option that makes the style configurable e.g. always, never and between 🤔.

Apart from that, we also need some unit tests to ensure the rule is working as expected and also to verify edge-cases like how does it behave when there are arbitrary statements between test functions or when test functions are on the same line?

Examples:

// everything in one line, does the rule behave correctly?
describe('', function () { it('', function () {}); it('', function () {}) });

// other code between two test functions, what should the rule do here?
describe('', function () {
  it('', function () {});
  const foo = 'bar';
  it('', function () {});
});

@bjornua
Copy link
Contributor Author

bjornua commented Jan 5, 2024

Seems like this is related to #145 and prior work has been done in #185 but it got never finished. Although #145 mentions a few other styling preferences like never having padding between test functions or always having them. Ideally we can support all of them in one rule and having an option that makes the style configurable e.g. always, never and between 🤔.

Adding spacing modes sounds interesting. I'm wondering if we (or someone motivated) could follow up on that in the future? between certainly seems like the most popular style and to me a sensible default.

Apart from that, we also need some unit tests to ensure the rule is working as expected and also to verify edge-cases like how does it behave when there are arbitrary statements between test functions or when test functions are on the same line?

Added test cases. Hopefully the workflows should pass now

@lo1tuma
Copy link
Owner

lo1tuma commented Jan 23, 2024

Sorry for the late response.

I'm wondering if we (or someone motivated) could follow up on that in the future?

I’ve thought about this and I like this approach. Although introducing more spacing options might require a different rule name which would be a breaking change. That’s being said, I’m willing to make this breaking change at some point. For now, it solves one specific use case and we can learn in future feature requests what exactly is needed.

Copy link
Owner

@lo1tuma lo1tuma left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants