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 property based tests on diff-sequences #9072

Merged
merged 8 commits into from
Oct 29, 2019
82 changes: 82 additions & 0 deletions packages/diff-sequences/src/__tests__/index.property.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import diff from '../';
import fc from 'fast-check'

const findCommonItems = (a: string[], b: string[]): string[] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I did, I'll retry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const array: string[] = [];
diff(
a.length,
b.length,
(aIndex: number, bIndex: number) => a[aIndex] === b[bIndex],
(nCommon: number, aCommon: number, bCommon: number) => {
for (; nCommon !== 0; nCommon -= 1, aCommon += 1) {
array.push(a[aCommon]);
}
},
);
return array;
};

const flatten = (data: string[][]) => {
const array: string[] = [];
for (const items of data) {
array.push(...items)
}
return array;
}

test('should be reflexive', () => {
dubzzz marked this conversation as resolved.
Show resolved Hide resolved
fc.assert(fc.property(
fc.array(fc.char()),
(a) => {
expect(findCommonItems(a, a)).toEqual(a)
}
))
})

test('should find the same number of common items when switching the inputs', () => {
// findCommonItems is not symmetric as:
// > findCommonItems(["Z"," "], [" ","Z"]) = [" "]
// > findCommonItems([" ","Z"], ["Z"," "]) = ["Z"]
fc.assert(fc.property(
fc.array(fc.char()), fc.array(fc.char()),
(a, b) => {
const commonItems = findCommonItems(a, b);
const symmetricCommonItems = findCommonItems(b, a);
expect(symmetricCommonItems).toHaveLength(commonItems.length)
}
))
})

test('should have at most the length of its inputs', () => {
fc.assert(fc.property(
fc.array(fc.char()), fc.array(fc.char()),
(a, b) => {
const commonItems = findCommonItems(a, b)
expect(commonItems.length).toBeLessThanOrEqual(a.length);
expect(commonItems.length).toBeLessThanOrEqual(b.length);
}
))
})

test('should be no-op when passing common items', () => {
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)
}
))
})

test('should find at the least the maximal known subset', () => {
fc.assert(fc.property(
fc.array(fc.array(fc.char())),
(data) => {
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
    }
  ))
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
))
})