Discussion: Use property-based tests

[ComradeVanti]

Currently tests operate on examples. Parameterized or property-based tests could offer additional confidence.

There are property-based testing libraries for many languages including for TS.

Property-based tests would not replace all tests, as often examples are an effective and sufficient approach, but it could augment and improve the test suite.

I would be willing to work on this. Thoughts?

[favoyang]

I haven't seen many real-world examples of these. It seems to be primarily beneficial for algorithm or arithmetic tests. My limited knowledge has hindered me from fully understanding its potential when applied to our test cases (most of them are based on string or JSON). Perhaps it can simplify verbose tests like test-editor-version or generate JSON for other tests.

I suggest you start with one or two types of unit tests in a PR first.

[ComradeVanti]

Alright, I'll make a few tests and let you know how it goes.

[ComradeVanti]

Working on this here: https://github.com/ComradeVanti/openupm-cli/tree/prop-test

[ComradeVanti]

@favoyang Take a look at 686baa7eae9fc7d642b272d721fed44ae0ea5b88
I have converted the tests for splitPkgNname. I was able to cover all possible inputs pretty quickly. I'm not sure I see too many immediate benefits, except it just feels more secure.

A possible drawback is that the implicit documentation that was present in the tests has now moved to the arbitraries file. What I mean is, that you can't tell from the tests that possible allowed versions are semantic versions, "latest" and some urls. You have to look at the arbitrary generating the version for that.

Let me know your thoughts.

[favoyang]

You're right; the tests have been moved to the attributes file. Furthermore, people will argue that we should move the properties definition as part of the docstring of the function definitions, and even the test cases themselves.

While writing dependent attributes might be challenging, however I think we can give it a try.

[ComradeVanti]

Im sorry, I don't think I understand. When you say attribute you mean arbitraries right? Are you suggesting we move arbitraries into the test files where they are used? That could work, but some arbitraries, such as semanticVersion will propbably be used by many tests so they need to be in a shared module.

[favoyang]

Im sorry, I don't think I understand. When you say attribute you mean arbitraries right?

My bad, it's a typo. I mean arbitraries.

Are you suggesting we move arbitraries into the test files where they are used? That could work, but some arbitraries, such as semanticVersion will propbably be used by many tests so they need to be in a shared module.

No, I just imagined the potential of the property-based tests. Because the arbitraries smell like a schema definition, it could be boxed into the function implementation as the docstring. And a modern test framework will unpack that for you. Too much magic perhaps.

/**
 * Split package-name, which may include a version into the actual name of the
 * package and the version if it exists
 * @param pkgName - oneof(@semanticVersion, "latest", @localPackageUrl, @httpPackageUrl, @gitPackageUrl) as @pkgName
 * @return - tuple(name: string, version: string)
 */
function splitPkgName(pkgName: string) 

Anyway, my response to this is a green light, based on your demo https://github.com/ComradeVanti/openupm-cli/commit/686baa7eae9fc7d642b272d721fed44ae0ea5b88

[ComradeVanti]

Ah, maybe there is potential there, but I think I would then rather attempt to use the type system for that. Ill take a look. Anyway, I'll convert a few more tests and let you know how it goes.

[ComradeVanti]

@favoyang would you mind if I also extract utility functions where relevant in order to improve test-ability or should I do that separately later?

[favoyang]

Let's put them in one PR for convenience.

[ComradeVanti]

After spending more time refactoring, I have come to the conclusion that, for now, property based tests add a lot of development effort for not much reward in our case. I have made a lot of other small refactors (see #60) but removed the prop tests. Maybe we can revisit the idea at some later time.

[favoyang]

Okay, I will review #60 instead.

I also tried the property-based approach, which saves duplicated code by merging unit tests into one test and moving the logic from the unit tests to the arbitraries files.

However, I find one major drawback: since the test fixture is described as an arbitrary object, sometimes I have to repeat myself to rewrite the logic to obtain an outcome. For example, consider a function that converts any GitHub URL to the HTTPS protocol.

function convertToGitHubHttps(githubUrl)

// define githubUrl arbitrary which is one of git://domain/reponame.git, https://domain.reponame
const githubUrl = fc.oneof(...

fc.property(githubUrl, (url) => {
  const expectedUrl = // have to implement a logic to get a outcome.
  const result= convertToGitHubHttps(url);
  return result === expectedUrl
})

In this example, to get the expectedUrl I basically re-implemented the convertToGitHubHttps which is the one I want to test.