refactor: Reorganize type definition test files

[dplewis]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

All definition tests are contained in single file. This is hard to maintain and isn't scalable.

Ref: #2012

Approach

• Allow for multiple tests files
• Use same test format as unit and integration tests
• Create a POC

Tasks

• Add tests
• Add changes to documentation (guides, repository pages, code comments)

[parse-github-assistant]

Thanks for opening this pull request!

• ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ef8be2c) to head (af36f96).
Report is 2 commits behind head on alpha.

Additional details and impacted files

@@            Coverage Diff            @@
##             alpha     #2488   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         6244      6244           
  Branches      1456      1468   +12     
=========================================
  Hits          6244      6244           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dplewis]

@mtrezza I added one example for Parse.Session since it's a small implementation. The community can addon more files in the future. An added benefit is we can ensure that all definitions are accounted for once the tests are migrated. In this case Parse.Session has all types tested. You can check by comparing it to the definition file ParseSession.d.ts

[mtrezza]

Why do we need to test types explicitly? Isn't that something that can be done automatically when types are defined in code?

[dplewis]

I believe we could have tested in code if the project was written in typescript from the beginning. Integration and Unit tests still use Common JS. There are two ESLint config files for this reason. There are a lot of weak types still need used, these definition test are for type correctness and type safety. I’m not sure which versions of Typescript we currently support.

I'm not a typescript expert so a second opinion would be preferred. Perhaps if we convert the entire repo to ES6 module like the push adapter we can remove these tests parse-community/parse-server-push-adapter#255

[mtrezza]

I don't think so. Converting the SDK to TS should already bring type safety to the SDKs code. The more parts of the SDK will be converted to TS, the more robust these checks should become. I'm referring to internal APIs, not just external APIs. Hence I wonder why we'd need to check for types in addition to that.

[dplewis]

To fix #1368, I had to copy the code to the type definition tests to see if it failed. Then I was able to fix the typing. Without the type tests this fix would have been far more difficult.

That issue was for dot notation in include but there are a lot of functions that accept dot notation that don't have test written for the yet.

[mtrezza]

For which methods / APIs do you suggest to add type tests?

[dplewis]

We already have type tests, this PR is to lay a foundation to organize those tests.

[mtrezza]

OK, but just for future contributions, for what parts of the code do you suggest we add type tests? Only public APIs?

[dplewis]

Only Public API

[dplewis]

@mtrezza I opened a issue for the flaky test #2530

[mtrezza]

Only Public API

Should we document that new requirement in the contribution guide, or is that already there?

[dplewis]

As always if it’s broken write a test for it. No need to document it. For example #2526 doesn’t need a test case unless someone finds a key that breaks it then we use types to restrict it or use code

[mtrezza]

So when adding a new public method, no type test needs to be added?

[dplewis]

Nope, we might never have to add another test again in a perfect world

[mtrezza]

I don't understand. You said earlier that type tests would be needed for public APIs but when adding a new public method it's not needed?

[mtrezza]

I suppose this has been closed in favor of #2538

[dplewis]

@mtrezza I figured that one file is enough no need to add extra deps.

[mtrezza]

Nice