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

Upgrade dependencies, switch Mocha tests from TS to JS #1318

Merged
merged 6 commits into from
Sep 5, 2024

Conversation

raucao
Copy link
Member

@raucao raucao commented Aug 9, 2024

Adds a new mocha suite for the RemoteStorage class as well, and also refactors some code.

The reason for using JavaScript in tests is that we cannot test the JavaScript usage of RS functions from TypeScript, when the test code doesn't compile due to mismatching types (i.e. when we validate wrong input from JS within the function). Also, it simplifies the Mocha setup and makes it more stable, since even the Mocha/TS combination is rather fragile and building blocks and setup hooks keep changing over time. This dependency upgrade wouldn't have worked with the existing setup for example.

refs #1270

@raucao raucao requested a review from a team August 9, 2024 09:45
@raucao
Copy link
Member Author

raucao commented Aug 20, 2024

@remotestorage/core If someone could have a quick look at this, it would be much appreciated. 🙏

@raucao
Copy link
Member Author

raucao commented Aug 31, 2024

Could still use a review for merging this...

@silverbucket
Copy link
Member

@raucao I'm a little confused by the rationale for switching from TS to JS for tests, when the RS code is written in TS. Can't we simply compile the tests to JS to run against the compiled JS code?

Copy link
Contributor

@DougReeder DougReeder left a comment

Choose a reason for hiding this comment

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

Looks good overall, but a few things are unclear; see line notes.

src/discover.ts Show resolved Hide resolved
test/unit/remotestorage-suite.js Show resolved Hide resolved
test/unit/remotestorage-suite.js Show resolved Hide resolved
@raucao
Copy link
Member Author

raucao commented Sep 1, 2024

@raucao I'm a little confused by the rationale for switching from TS to JS for tests, when the RS code is written in TS. Can't we simply compile the tests to JS to run against the compiled JS code?

The compilation fails if you pass the wrong type, so you cannot test how the library handles bad input.

@silverbucket
Copy link
Member

The compilation fails if you pass the wrong type, so you cannot test how the library handles bad input.

You can use different typing techniques to account for that.

rs.myMethd(badObj as ProperObj) for example.

Or you can go even more extreme, with (badObj as unknown as ProperObj).

Things like that usually work for 90% of cases. For more extreme cases, there are ways to work around it but maybe in a less generic way that described above.

@raucao
Copy link
Member Author

raucao commented Sep 2, 2024

Things like that usually work for 90% of cases. For more extreme cases, there are ways to work around it but maybe in a less generic way that described above.

The point is that there's no need to work around anything for this use case (both for new and existing contributors), and it also fixes dependency issues with Mocha and TypeScript at the same time. Types keep the source code safe and easy to reason about, but have honestly just been in the way most of the time for tests, without enjoying the same benefits.

@DougReeder
Copy link
Contributor

When I run test:mocha, the tests all pass, but I get the message https%3A%2F%2Fnote.app.com%2F&scope=notes%3Arw&client_id=opaque&state=CSRF-protection&response_type=token&code_challenge=ABCDEFGHI&code_challenge_method=plain and the process doesn't exit. Is that an inescapable side effect of testing the timeout?

@raucao
Copy link
Member Author

raucao commented Sep 2, 2024

Is this happening on Node.js 18 and 20? Testing timeouts is indeed a bit tricky, because there's no AbortController for Node's fetch. This can cause the test run to look finished, but take another few seconds to exit after that.

@DougReeder
Copy link
Contributor

Sometimes the tests exit, sometimes they don't. This is using node v18.18.2, v20.11.1 and v21.7.3 under MacOS Sonoma 14.6.1

@raucao
Copy link
Member Author

raucao commented Sep 4, 2024

And it's not happening on master? I.e. it's definitely something caused by the changeset here?

@DougReeder
Copy link
Contributor

And it's not happening on master? I.e. it's definitely something caused by the changeset here?

I'm observing this sometimes on master as well, so it doesn't appear to be anything new.

Copy link
Contributor

@DougReeder DougReeder left a comment

Choose a reason for hiding this comment

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

LGTM

@raucao raucao merged commit c0e9596 into master Sep 5, 2024
2 checks passed
@raucao raucao deleted the chore/upgrade_dependencies branch September 5, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants