Add property based tests on diff-sequences #9072

dubzzz · 2019-10-21T21:27:47Z

Summary

When I added the first property based tests in Jest codebase - see #8012 - @pedrottimark asked me:

Your thoughts also welcome about tests for diff-sequences package in Jest repo.

This PR adds property based tests to check diff-sequences.

I think we should find better namings for some of the tests I added 🤔

Test plan

PR only adds tests.

pedrottimark · 2019-10-21T21:40:48Z

packages/diff-sequences/src/__tests__/index.property.test.ts

+      const allData = flatten(data); // [...data[0], ...data[1], ...data[2], ...data[3], ...]
+      const partialData = flatten(data.filter((_, i) => i % 2 === 1)) // [...data[1], ...data[3], ...]
+      const commonItems = findCommonItems(allData, partialData)
+      expect(commonItems.length).toBeGreaterThanOrEqual(partialData.length)


This test also passes with toEqual which I think makes it similar to the preceding test. What do you think?

Exploring what it would look like to use the preceding test alone to assert the property:

it('should find subsequence', () => { fc.assert(fc.property( fc.array(fc.char()), fc.array(fc.char()), (a, b) => { const commonItems = findCommonItems(a, b) expect(findCommonItems(a, commonItems)).toEqual(commonItems) expect(findCommonItems(commonItems, a)).toEqual(commonItems) expect(findCommonItems(b, commonItems)).toEqual(commonItems) expect(findCommonItems(commonItems, b)).toEqual(commonItems) } )) });

If I am right your suggestion for should find subsequence is close to should be no-op when passing common items: it adds the b-based cases.

💭 Let's suppose for a minute that we remove the test called should find at the least the maximal known subset...

In that case if we use findCommonItems = (a, b) => [] all the properties will pass. None of them are checking that there are at least some items - because they actually cannot do so given the few assumptions they have on the inputs.

Actually this last test is the only test covering the we expect at least N entries in the list of common items

Update: should be reflexive would be error in the scenario described above

What do you think if the assertion becomes: expect(commonItems).toEqual(partialData)

You're totally right as we have both:

expect(commonItems.length).toBeGreaterThanOrEqual(partialData.length)

expect(commonItems.length).toBeLessThanOrEqual(partialData.length) // rule we use for 'should have at most the length of its inputs'

We have: expect(commonItems).toEqual(partialData)

It makes this it much more useful as it produces sets of entries for which we know what are the exact outputs.

You can ignore my comment:

It might even replace my hacky test: should find at the least the maximal known subset

#9072 (comment)

Yes, in the last test, let‘s assert the items themselves instead of the length.

If you agree in the second to last test, let‘s include assertions for both a and b and replace “no-op” in name with something more descriptive

packages/diff-sequences/src/__tests__/index.property.test.ts

pedrottimark · 2019-10-21T21:43:42Z

packages/diff-sequences/src/__tests__/index.property.test.ts

+import diff from '../';
+import fc from 'fast-check'
+
+const findCommonItems = (a: string[], b: string[]): string[] => {


lint-and-typecheck failed on CI with Array type using 'string[]' is forbidden. Use 'Array<string>' instead.

Thank you. It looks like a few more lint errors. Does yarn lint --fix locally display them?

I thought I did, I'll retry

@pedrottimark This time I did it for sure ^^ I might not be at head. Maybe I could just merge onto master on branch to get the latest changes

Ohhh, now it’s about the Facebook comment at the beginning of the file. I always forget it.

You can copy and paste from index.test.ts file

Got it: Facebook copyright header check failed for the following files

pedrottimark · 2019-10-22T13:46:34Z

Nicolas, thank you for this practical tutorial on property-based testing!

Beyond the scope of this pull request, is there a generic name for a test that compares the package to a short but slow implementation of diff for random inputs?

dubzzz · 2019-10-22T20:51:11Z

Beyond the scope of this pull request, is there a generic name for a test that compares the package to a short but slow implementation of diff for random inputs?

Such test would be pretty useful - testing the optimal implementation against a dummy one that just does the job. It might even replace my hacky test: should find at the least the maximal known subset.

I have no generic naming for it. But we can name it something like:
should produce the same output as a non optimal reference *reference or implementation

pedrottimark · 2019-10-22T21:13:58Z

Super, I will find my quadratic implementation in JavaScript and convert to TypeScript

Do you recommend (in future pull request) that I add it to this test file or add a separate test file?

dubzzz · 2019-10-22T21:20:02Z

Do you recommend that I add it to this test file or separate test file in future pull request?

As you like ;) But really good idea to add this one ;)

If we want to go further we can even go further by adding yet another test:

generate a and b fully random arrays of chars
compute common items
count the number of each char in common, same for a (and b)
assert that for any character in common the number of times this character appears in common is less or equal to the number of times it appears in a

Can be seen as a variant of should have at most the length of its inputs

…uts`

dubzzz · 2019-10-22T21:45:59Z

I added the test I was talking about in my previous comment here: 5c2e725

pedrottimark · 2019-10-25T19:33:21Z

After some time in my mental slow cooker, I remembered some pieces that might interest you:

index.test.ts has an alternative algorithm as countDifferences because, as you commented in the symmetric test, you can test for the number but not the items themselves
for randomized sequences, you can test that common items are subsequence of a and b

const isSubsequenceOf = (
  subsequence: Array<string>,
  sequence: Array<string>,
): boolean => {
  let iSub = 0;
  let iSeq = 0;

  while (iSub !== subsequence.length && iSeq !== sequence.length) {
    if (subsequence[iSub] === sequence[iSeq]) {
      iSub += 1;
    }
    iSeq += 1;
  }

  return iSub === subsequence.length;
};

Would you like to add a test of random sequences which asserts

common items have expected length computed by alternative algorithm
common items are indeed a subsequence of both sequences

Because the last test constructs a subsequence of a sequence, it can replace assertion about number of items with assertion about the items themselves:

expect(commonItems).toEqual(partialData);

packages/diff-sequences/src/__tests__/index.property.test.ts

codecov-io · 2019-10-27T19:11:32Z

Codecov Report

Merging #9072 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9072      +/-   ##
==========================================
- Coverage   64.07%   64.05%   -0.03%     
==========================================
  Files         277      277              
  Lines       11681    11676       -5     
  Branches     2863     2862       -1     
==========================================
- Hits         7485     7479       -6     
  Misses       3572     3572              
- Partials      624      625       +1

Impacted Files	Coverage Δ
packages/jest-reporters/src/get_result_header.ts	`33.33% <0%> (-14.29%)`	⬇️
packages/expect/src/utils.ts	`94.87% <0%> (-0.07%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd48ccc...03d6b64. Read the comment docs.

pedrottimark

Thanks again! This looks ready to merge if you are happy with it.

dubzzz · 2019-10-29T17:55:03Z

I'm happy with it, we can go ahead

github-actions · 2021-05-11T14:06:43Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

Add property based tests on diff-sequences

2ada316

facebook-github-bot added the cla signed label Oct 21, 2019

pedrottimark reviewed Oct 21, 2019

View reviewed changes

packages/diff-sequences/src/__tests__/index.property.test.ts Outdated Show resolved Hide resolved

pedrottimark reviewed Oct 21, 2019

View reviewed changes

Fix lint and switch from test() to it()

9360d9c

Run lint fix and adapt according to comments

dd3ccfd

dubzzz added 2 commits October 22, 2019 23:26

Add missing facebook header

f281006

Add `should have at most the same number of each character as its inp…

5c2e725

…uts`

Add test for subsequence

ed21dbb

pedrottimark reviewed Oct 26, 2019

View reviewed changes

packages/diff-sequences/src/__tests__/index.property.test.ts Outdated Show resolved Hide resolved

dubzzz added 2 commits October 27, 2019 18:31

Fix isSubsequenceOf

8770811

Fix isSubsequenceOf again (and check tests are still green)

03d6b64

pedrottimark approved these changes Oct 29, 2019

View reviewed changes

pedrottimark merged commit bf61095 into jestjs:master Oct 29, 2019

dubzzz deleted the pbt-diff-sequences branch October 30, 2019 22:43

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add property based tests on diff-sequences #9072

Add property based tests on diff-sequences #9072

dubzzz commented Oct 21, 2019

pedrottimark Oct 21, 2019

dubzzz Oct 21, 2019

dubzzz Oct 22, 2019

pedrottimark Oct 22, 2019

dubzzz Oct 22, 2019

dubzzz Oct 22, 2019

pedrottimark Oct 22, 2019

pedrottimark Oct 21, 2019

dubzzz Oct 21, 2019

pedrottimark Oct 22, 2019

dubzzz Oct 22, 2019

dubzzz Oct 22, 2019

pedrottimark Oct 22, 2019

dubzzz Oct 22, 2019

pedrottimark commented Oct 22, 2019 •

edited

Loading

dubzzz commented Oct 22, 2019

pedrottimark commented Oct 22, 2019 •

edited

Loading

dubzzz commented Oct 22, 2019 •

edited

Loading

dubzzz commented Oct 22, 2019

pedrottimark commented Oct 25, 2019

codecov-io commented Oct 27, 2019

pedrottimark left a comment

dubzzz commented Oct 29, 2019

github-actions bot commented May 11, 2021

Add property based tests on diff-sequences #9072

Add property based tests on diff-sequences #9072

Conversation

dubzzz commented Oct 21, 2019

Summary

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrottimark commented Oct 22, 2019 • edited Loading

dubzzz commented Oct 22, 2019

pedrottimark commented Oct 22, 2019 • edited Loading

dubzzz commented Oct 22, 2019 • edited Loading

dubzzz commented Oct 22, 2019

pedrottimark commented Oct 25, 2019

codecov-io commented Oct 27, 2019

Codecov Report

pedrottimark left a comment

Choose a reason for hiding this comment

dubzzz commented Oct 29, 2019

github-actions bot commented May 11, 2021

pedrottimark commented Oct 22, 2019 •

edited

Loading

pedrottimark commented Oct 22, 2019 •

edited

Loading

dubzzz commented Oct 22, 2019 •

edited

Loading