Jest: Consider allowing skipped tests

ESLint currently complains about tests skipped with xit or it.skip, giving the impression that we don't want skipped tests at all. Overriding this lint flags the file with a code smell even if skipping a test is justified. Disallowing skipped tests may encourage devs to comment them out or delete them altogether.

Why are skipped tests useful?

Sometimes, tests break, give false positives, require future work, or stop representing the current state of their component. An example is in spec/frontend/notes/old_notes_spec.js where a test started giving a false positive even though the things it was testing were missing. This test should be flagged, but because of ESLint I commented it out instead, dooming the test to eternal neglect.

Advantages of allowing skipped tests

  • Skipped tests appear in the test results, and are immediately visible to devs.
  • It's an easy way to quarantine tests in Jest.
  • They are more easily grep-ed than commented-out tests.
  • They can be integrated into Danger to warn devs if their MRs increase the number of skipped tests.
  • We can use skipped tests as a metric to assess code quality and health of our test code
  • We can use skipped tests to track of our performance in the Karma -> Jest migration.
    • If we automate Karma -> Jest migration, we can use skipped tests to flag unmigratable tests for review, commit them, and iterate later.

Disadvantages

  • If the number of skipped tests becomes too big (absolute value? skipped/passing ratio?), someone needs to yell at devs to finally fix them.
  • We need to write logic to track skipped tests.
  • ?

Discuss!

@winh

Edited Jun 03, 2019 by Martin Hanzel
Assignee Loading
Time tracking Loading