Deduplicate testing code - with typeclasses #199

milesfrain · 2021-04-22T01:44:20Z

Description of the change

This PR demonstrates another testing strategy that accomplishes the following:

Encourages API consistency
- It lets the compiler help us fix Patch API differences #183 and prevent future API gaps.
Reuses tests
- Common tests were previously duplicated 4+ times for the different list types.
- This strategy could be extended to allow reuse across a variety of container/collection types.
  - It looks like many of the tests in this lists library were initially copied over from the arrays library, so it would be great if we could let these be reused when possible.

This new testing strategy involves creating testing-specific typeclasses that cluster common APIs. This lets the compiler enforce common interfaces during library development, but doesn't expose this misuse of typeclasses to library consumers (see #184 for more discussion on typeclasses as interfaces).

These changes are additive at the moment, but we could also delete any duplicated tests as they are migrated to these common tests. The goal is to migrate all preexisting tests.

Progress:

Typeclass interfaces for full API coverage (see proposed API list in Patch API differences #183 (comment)). Note that some "common" APIs may be split across multiple typeclasses due to non-identical type signatures.
unsafeCrashWith placeholders for missing APIs.
Copy over existing tests or create new tests for full API coverage.
Enhancement: Convert assert to assertEqual for better printing of testing mismatches.

PR Checklist:

Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
Linked any existing issues or proposals that this pull request should close
Updated or added relevant documentation
Added a test for the contribution (if applicable)

milesfrain · 2021-04-22T01:46:40Z

test/Test/Common.purs

+  concat :: forall a. c (c a) -> c a
+  concatMap :: forall a. forall b. (a -> c b) -> c a -> c b
+  -- Should basic list have a cons function wrapping the Cons constructor?
+  cons :: forall a. a -> c a -> c a
+  elemIndex :: forall a. Eq a => a -> c a -> Maybe Int
+  elemLastIndex :: forall a. Eq a => a -> c a -> Maybe Int
+  findIndex :: forall a. (a -> Boolean) -> c a -> Maybe Int
+  findLastIndex :: forall a. (a -> Boolean) -> c a -> Maybe Int
+  foldM :: forall m a b. Monad m => (b -> a -> m b) -> b -> c a -> m b
+  index :: forall a. c a -> Int -> Maybe a
+  intersect :: forall a. Eq a => c a -> c a -> c a
+  intersectBy :: forall a. (a -> a -> Boolean) -> c a -> c a -> c a
+  length :: forall a. c a -> Int
+  nubEq :: forall a. Eq a => c a -> c a
+  nubByEq :: forall a. (a -> a -> Boolean) -> c a -> c a
+  reverse :: c ~> c
+  singleton :: forall a. a -> c a
+  snoc :: forall a. c a -> a -> c a
+  toUnfoldable :: forall f a. Unfoldable f => c a -> f a
+  union :: forall a. Eq a => c a -> c a -> c a
+  unionBy :: forall a. (a -> a -> Boolean) -> c a -> c a -> c a
+  -- Types don't have to be all a
+  -- Todo - double check this requirement
+  unzip :: forall a b. c (Tuple a b) -> Tuple (c a) (c b)
+  zip :: forall a b. c a -> c b -> c (Tuple a b)
+  zipWith :: forall a b d. (a -> b -> d) -> c a -> c b -> c d
+  zipWithA :: forall a b d m. Applicative m => (a -> b -> m d) -> c a -> c b -> m (c d)


Here's the example of APIs that are available to all list types. There should probably be more added here.

Are we certain that all "list" types can (or should) implement the same APIs? It seems that at least some operations would be useful for some list types and not others -- for example, I'd imagine some operations don't make sense for a lazy list.

I don't know the best place to have this discussion -- perhaps in the related "Patch API differences" conversation -- but my gut sense is that abstracting over list types in this way may have undesirable consequences for a) performance and b) error messages, and that we need to weigh that against the advantages of a unified interface.

Actually -- I see that this is a type class specifically for testing these libraries, so the performance and error messages don't matter here. But I'd still like to hear your thoughts about what we should do when a library needs to diverge from the standard API -- how would you envision us handling that?

Are we certain that all "list" types can (or should) implement the same APIs? It seems that at least some operations would be useful for some list types and not others -- for example, I'd imagine some operations don't make sense for a lazy list.

You are correct. I just linked to the most "universally common" snippet, but there are testing clusters in this PR that target just Lazy vs "Strict" lists or "Emptiable" vs "NonEmpty" lists. I think looking at the UpdatedTests.purs file in this PR is a good place to see how everything fits together.

my gut sense is that abstracting over list types in this way may have undesirable consequences for a) performance and b) error messages, and that we need to weigh that against the advantages of a unified interface.

This abstraction is just for managing and running tests, so shouldn't affect end users of the library.
But regarding error messages during testing, I'd love to be able to use test-unit (or similar) instead of assert so we can more easily see what's causing the failure.

I'd love to be able to use test-unit (or similar) instead of assert so we can more easily see what's causing the failure.

Core libraries only depend on other core libraries, so improvements to the testing situation would have to be added to assert; we won't be able to depend on other testing libraries, unfortunately.

I'd still like to hear your thoughts about what we should do when a library needs to diverge from the standard API -- how would you envision us handling that?

Currently, this is just a common API for this lists library, but to answer two related questions:

What if one of the sub-list types (e.g. LazyNonEmpty) needs to diverge from the common List API?

In that case, the offending function is no longer common across all list types, and can be broken-out into whatever cluster that function is still common among.

What if we use a common API among multiple containers (e.g. Arrays and Lists) and some List functions need to diverge.

Similar answer to the above. If those functions aren't common across all containers, then they can't be part of the common-container API, but they can be demoted to part of the common-list API.

I'd love to be able to use test-unit (or similar) instead of assert so we can more easily see what's causing the failure.

I forgot we had assertEqual, which works fine for displaying diffs.

milesfrain · 2021-04-22T03:02:25Z

Having issues reproducing the CI failure locally. Don't know why. I believe versions are the same:

$ pulp --version        
Pulp version 15.0.0
purs version 0.14.1 using /home/miles/.nvm/versions/node/v14.9.0/bin/purs

$ pulp build -- --censor-lib --strict --censor-codes='UserDefinedWarning'
* Building project in /home/miles/projects/purescript/lists
Invalid option `--censor-lib'
Usage: purs COMMAND
  The PureScript compiler and tools
* ERROR: Subcommand terminated with exit code 1

thomashoneyman · 2021-04-22T04:29:28Z

Looks like you don’t have psa installed — you can either install it or omit the flags passed to pulp. The error you’re seeing is because of a compiler warning introduced in 0.14.1 that warns on unused names.

Migrating common tests on the todo list

Tests todo

Applied some other renaming todos

proof of concept

49acc40

milesfrain commented Apr 22, 2021

View reviewed changes

milesfrain mentioned this pull request Apr 22, 2021

Patch API differences #183

Open

milesfrain added 7 commits April 22, 2021 11:38

fix CI

45dc68e

Added more common APIs and placeholders

e3043d0

Migrating common tests on the todo list

remove placeholder templates

9a190dd

add common-ish alterAt deleteAt

ef11865

All crashWith placeholders added

0e0efe2

Tests todo

convert common tests to assertEqual

c6bcc03

Limit specialized replicate* to Lazy

dcf14e8

Applied some other renaming todos

milesfrain mentioned this pull request May 24, 2021

Deduplicate testing code - with records #200

Draft

milesfrain changed the title ~~Deduplicate testing code~~ Deduplicate testing code - with typeclasses May 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate testing code - with typeclasses #199

Deduplicate testing code - with typeclasses #199

milesfrain commented Apr 22, 2021 •

edited

Loading

milesfrain Apr 22, 2021

thomashoneyman Apr 22, 2021 •

edited

Loading

thomashoneyman Apr 22, 2021 •

edited

Loading

milesfrain Apr 22, 2021

thomashoneyman Apr 22, 2021

milesfrain Apr 22, 2021

milesfrain Apr 26, 2021

milesfrain commented Apr 22, 2021 •

edited

Loading

thomashoneyman commented Apr 22, 2021

Deduplicate testing code - with typeclasses #199

Are you sure you want to change the base?

Deduplicate testing code - with typeclasses #199

Conversation

milesfrain commented Apr 22, 2021 • edited Loading

milesfrain Apr 22, 2021

Choose a reason for hiding this comment

thomashoneyman Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

thomashoneyman Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

milesfrain Apr 22, 2021

Choose a reason for hiding this comment

thomashoneyman Apr 22, 2021

Choose a reason for hiding this comment

milesfrain Apr 22, 2021

Choose a reason for hiding this comment

milesfrain Apr 26, 2021

Choose a reason for hiding this comment

milesfrain commented Apr 22, 2021 • edited Loading

thomashoneyman commented Apr 22, 2021

milesfrain commented Apr 22, 2021 •

edited

Loading

thomashoneyman Apr 22, 2021 •

edited

Loading

thomashoneyman Apr 22, 2021 •

edited

Loading

milesfrain commented Apr 22, 2021 •

edited

Loading