search

facebook / prop-types

Runtime type checking for React props and similar objects
MIT License
4.48k stars 356 forks source link

Environment testers #291

Open conartist6 opened 5 years ago

conartist6 commented 5 years ago

Sorry this I couldn't get the PR to show up quite the way I wanted. The intent is to reorganizing the tests so that all the test code isn't written four times. You can see how that works by looking at this file: https://github.com/plangrid/prop-types/blob/environment-testers/__tests__/arrayOf-test.js

The tests are written in a callback which is then run once for each environment. The results look like this: Screen Shot 2019-08-30 at 3 53 21 PM

Note that one outstanding issue is that the describe text for each test isn't completely accurate, but that is already the case with the copy/paste strategy.

conartist6 commented 5 years ago

Primarily I wish to know if a change like this would be merged. If it would not be I don't want to spend my time moving the rest of the code around.

conartist6 commented 5 years ago

I'm not saying there's no cost, but I'm pretty sure the benefit is worth it. Though it is difficult to tell, each of those existing test files tests arbitrary and subtly different things. It's 4x harder to write meaningful test code for any method, since most dev time is spent copying and pasting and working with the subtle differences of what goes where. Do I put the exact same code in each of the 4 files, just with different helpers? Many existing validator tests seem to feel that it isn't as important to test certain types of functionality in certain configurations. As a user of the package I just want to know that all the possible usages work as expected for each possible import style. Why would it not be valuable for the test code to be structured to match that expectation?

conartist6 commented 5 years ago

@ljharb Could you help me any further? What would I have to do to convince you? If you can't be convinced, can you resolve for me whether the intention is to test all use cases in all environments, and if it isn't, I would like to know what the criteria for a usage being tested in a particular environment is.

ljharb commented 2 years ago

@conartist6 since you made this PR from a fork that's not your own, i don't think i'll be able to rebase this for you; can you rebase this?