Skip to content

Add basic tester Github workflow#16

Closed
2colours wants to merge 3 commits intosquirrelchat:mistressfrom
2colours:mistress
Closed

Add basic tester Github workflow#16
2colours wants to merge 3 commits intosquirrelchat:mistressfrom
2colours:mistress

Conversation

@2colours
Copy link
Copy Markdown

Relates to #7

As I investigated the test failure of my package with Windows, I wanted to confirm I didn't inherit the failure from this dependency (spoiler: this package tests just fine under Windows). Since this workflow was largely just a matter of copying and pasting and it's definitely better than nothing, why not share it? :)

Currently, it installs the (dev) dependencies of the package and runs the tests on all configurations of 3 OSes (latest Windows, latest MacOS, latest Ubuntu) and the 3 latest stable Node versions (v16 is the latest that works with this package and/or the dev dependencies). The tests pass on all these configurations.

Signed-off-by: Márton Polgár <37218286+2colours@users.noreply.github.com>
Signed-off-by: Márton Polgár <37218286+2colours@users.noreply.github.com>
Signed-off-by: Márton Polgár <37218286+2colours@users.noreply.github.com>
@cyyynthia cyyynthia self-requested a review April 8, 2024 20:11
@cyyynthia cyyynthia added the enhancement New feature or request label Apr 8, 2024
@cyyynthia
Copy link
Copy Markdown
Member

Hey there, thanks for the PR and sorry for the delay.

I'm not sure testing on all 3 platforms is relevant for the case of smol-toml. It involves no platform-specific code and is pretty much agnostic to the operating system you're on, so I'm unsure if Windows and MacOS jobs are relevant.

As for the Node version matrix, Node 16 never was supported by this library explicitly (see the package.json versions field), so it shouldn't be a version the lib tests against. It even reached EOL a while ago, so nobody should be using it anymore and migrate to supported versions of Node.

The tests wrote also are not quite complete; they're a good starting point but it'd need to be tested with toml-test for a more thorough testing. Good starting point nonetheless!

@cyyynthia cyyynthia removed their request for review April 9, 2024 19:59
@2colours
Copy link
Copy Markdown
Author

2colours commented Apr 12, 2024

EDIT: oops, that was a weird misclick. 😅

I'm not sure testing on all 3 platforms is relevant for the case of smol-toml. It involves no platform-specific code and is pretty much agnostic to the operating system you're on, so I'm unsure if Windows and MacOS jobs are relevant.

I think this reasoning is a bit akin to something like "I have already tested this library so why run them again". If it's naturally supposed to work on other platforms, to me that's one less excuse for not testing on all of them or writing tests that are less multiplatform than the running software itself.

As for the Node version matrix, Node 16 never was supported by this library explicitly (see the package.json versions field), so it shouldn't be a version the lib tests against. It even reached EOL a while ago, so nobody should be using it anymore and migrate to supported versions of Node.

I should have checked this, maybe I didn't notice but anyway, if it shouldn't install on Node v16 and it does, I would say that's legitimate feedback either way. Either something went wrong in the workflow or it does install with v16 disregarding the metadata - in which case I for one don't think it's the task of library maintainers to deliberately gatekeep outdated versions of Node. There is a nuanced line between supporting something and just letting something work - it's good to know that this library can work in certain environments, regardless whether that's intentional or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants