feat: More flexible missing-expect rule

[AdrienLemaire]

Feature Request

Make missing-expect smarter by detecting expect statements in helper functions.

Problem || Goal

An expect can become verbose, and moving it to an helper function or class method make for tests easier to read and maintain

Example page

class Hub {
  title: Selector;

  constructor() {
    this.title = main.find("h1").withText("Village Hub");
  }

  async isOpen() {
    // Verify that we are on the hub page
    t.expect(this.title.exists).ok("We have landed on the Hub page", {
      // it takes a while to go from the firebase popup to the hub page when signin
      // from the Top page
      timeout: 5000,
    });
  }
}

Test:

// Scenario outline: A user can signin from different entrypoints
Object.entries(entrypoints).forEach(
  ([path, [signinButton, providerButton]]) => {
    test(`Signin flow: ${path}`, async (t) => {
      // Open signin modal and click the user's provider button
      await t.click(signinButton).click(providerButton);

      await fillCredentials(user.email, user.password);
      await Hub.isOpen();
    });
  }
);

The test above raises an eslint warning Please ensure your test has at least one expect, althought it has one in the isOpen method.

This is because the rule searches for the exact term expect in the test code in https://github.com/testcafe-community/eslint-plugin-testcafe-community/blob/master/lib/rules/missing-expect.ts#L102

Expected behavior

No warning should be raised.

Potential solutions

Not sure about the effort required to open helpers / class methods and parse their code. An alternative would be to add an eslint parameter to specify alternate expect statements

rules:
  testcafe-community/missing-expect:
    - aliases:
      - isOpen

[codejedi365]

@AdrienLemaire, thank you for the great write up and example information. I definitely can see how this could be problematic if the assertions are abstracted out of the test function scope.

I will have to do some more research on the internals available within ESLint because of the traditional Abstract Syntax Tree (AST) and the hook mechanism don't actually compile/load the code to allow for "following" of functions. As my understanding goes, ESLint isolates the current file that is being linted which means following external (imported) files would be out-of-scope. I estimate it would need to somehow follow the import and run an AST on the secondary file and spider out from there, which might not be doable with adequate performance.

Your alternative of aliased expect functions is a great recommendation and I think that would be much simpler to implement at the moment. I am curious if you would have any side-effects from a generic isOpen or would it need to specify the object too (ie Hub.isOpen)? I realize the rule does not currently check the root object t of t.expect(), but it has been in deliberation of a future implementation in order to elevate false positives.

Separately, I want to offer my opinion on test structure in case that might stem a different idea or direction for you and your team. I have debated the idea of abstractions related to assertions due to the extension of the idea of a PageModel but I don't think it fits with most recommendations for testing paradigms. From things I have read/watched, tests can be difficult to debug with variables being abstracted away or reused. These sources tended to forgo the DRY paradigm in order to ensure self contained instructions for their tests. The fear was of diverging tests with intersecting function dependencies. I do support the PageModel in order to test behaviors of a webpage instead of the specifics of what is clicked but I personally drew the line at ensuring assertions stayed in the test functions as an assertion doesn't make sense outside of a test(). I recognize the Object-Oriented desire for using assertions via PageModel objects with isAttr() type functions because it follows most other OO design structures, although I don't think TestCafe recommended assertion inclusion. I would see your test looking like this instead:

// hub.test.js
import Hub from "./page-models/hub"

// global constants, For test readability
MILLISECONDS = 1
SECONDS = 1000 * MILLISECONDS

// it takes a while to go from the firebase popup to the hub page when signing-in
// from the Top page
SIGN_IN_THRESHOLD = 5 * SECONDS

test(`Signin flow: ${path}`, async (t) => {
    // Open signin modal and click the user's provider button
    await t.click(signinButton).click(providerButton);

    await fillCredentials(user.email, user.password);

    // NOTE: string provided is for when the test fails.  Your msg looks like a success message to me.
    // function signature = t.expect().ok(errMsg, opts)
    // from https://testcafe.io/documentation/402716/reference/test-api/testcontroller/expect/ok
    // message = "An assertion message displayed in the report if the test fails"

    await t.expect(Hub.title.exists).ok("Failed to reach hub page", {
        timeout: SIGN_IN_THRESHOLD
    });
});

Thank you for indulging my discourse and I recognize test structure is up to your team. I'm glad I thought about this topic again as I notice your test Hub.isOpen() does read nicer than an assert statement does.

[AdrienLemaire]

@codejedi365 thank you for the great reply and side notes. One reason I love OSS is thanks to contributors like you.

I suppose we'll keep our expects in test functions for the time being, and we'll follow the TestCafe recommendations.

It is fair enough to close this issue as "wontfix because encourages bad practices", and maybe mentioning this good practice in the doc could be useful to people like me :+1: