Add Tests and CI #17

wardellbagby · 2022-12-16T05:05:50Z

This PR adds tests and continuous integration to this project. The commits themselves break down what they do better than I could summarize here but effectively, we add tests and make changes to the library that were necessary to make those tests possible, while also trying to modify the repo as little as possible. And a little bit of CI, as a bonus since I was already here.

I don't believe I've changed any functionality here but the logic around the changing of return to process.exit(1) is technically different behavior because process.exit(1) will always quit while return might quit. I explain the why of that change in the first commit as well.

wardellbagby · 2022-12-16T05:07:50Z

fetch.js

@@ -1,6 +1,6 @@
 const https = require("https");

-module.exports = function fetch(options, data) {
+module.exports = function fetch(options) {


The "data" parameter looked unused by all usages of this function. I figured that was for eventual tests, so I removed it here since I did add tests.

Hmm, looks like the original author used data as a variable/object to hold fetch results output? https://github.com/dev-drprasad/delete-tag-and-release/blob/master/index.js#L76
But perhaps it could have just been named something more commonly seen like results? dunno what his ideas were, do you looking through that?

Yeah, I think I figured out what happened there. data was meant for actually sending data along with the request. I'm going to un-delete that since it's not actually relevant to what I wanted to do: namely add tests.

wardellbagby · 2022-12-16T05:11:52Z

utils.js

+
+// The fact that this uses the "createGetReleasesPath" is used in a test, so changes here should be reflected there as
+// well.
+const createDeleteReleasesPath = (releaseId, fullyQualifiedRepo = getRepoFromProcess()) => `${createGetReleasesPath(fullyQualifiedRepo)}/${releaseId}`


The idea of fullyQualifiedRepo is that it's the owner plus the name of a repo, since that always uniquely identifies a GitHub repo. I liked that idea so it's used pretty extensively by the tests but to keep the changes as small as possible, I didn't change the actual Action code to use that same pattern, but we definitely could. owner and repo in lib.js are never used independently; only together. Makes sense to just combine them once and never bother with it again.

That being said, it should always be explicitly provided by lib.js when it calls one of the create functions in here; lib.js relying on getRepoFromProcess would be bad since the process.env can be changed during the Action's run. We don't care in tests, though since that would just fail the test.

I think we can go ahead and change the Action code. I'd likely go for good testing (as you have done), and then we can do a prerelease of the action itself which signifies to community that it's not production ready (and so they can test and also give feedback).

The other thing is that I want to bump to eventually (maybe with all these changes) a major version like v2.x instead of the current v0.2.1 since the Action has indeed been used by thousands of repos now from our insights and the semver would be easier understood with a real number against MAJOR.MINOR.PATCH

Thoughts?

Yeah, I think bumping would be good. But not to v2.x, but v1.x. Bumping to 2.x.x isn't actually semver compatible; as far as I remember, all bumps have to be incremental by 1. So would have to bump to v1.0.0.

It's semver compatible since semantic versioning doesn't say major version incremental by 1. It says each element must increase numerically. And for major version, if any backwards incompatible changes are introduced, it must be incremented; it doesn't say HOW much ;-). This has been brought up in the past, and covered in some of the FAQ Q/A like this one and a few others there: https://semver.org/#what-if-i-inadvertently-alter-the-public-api-in-a-way-that-is-not-compliant-with-the-version-number-change-ie-the-code-incorrectly-introduces-a-major-breaking-change-in-a-patch-release

but maybe "by 1" would indeed be less confusing for the uninitiated, so we'll do that way when the time comes; agree.

thadguidry · 2023-03-20T01:20:27Z

@wardellbagby Hi again! Could you resolve the latest conflicts? And also, is your branch testable yet by me or someone else? or is it still somewhat of a WIP (work in progress) or Draft?

wardellbagby · 2023-03-20T01:50:34Z

Hey @thadguidry! Yeah, I'll resolve the conflicts in a bit.

Yes, it's testable and once I resolve conflicts, it'll be mergable.

You can run the tests via npm run test.

package.json

thadguidry · 2023-03-20T01:55:40Z

utils.js

+
+// The fact that this uses the "createGetReleasesPath" is used in a test, so changes here should be reflected there as
+// well.
+const createDeleteReleasesPath = (releaseId, fullyQualifiedRepo = getRepoFromProcess()) => `${createGetReleasesPath(fullyQualifiedRepo)}/${releaseId}`


I think we can go ahead and change the Action code. I'd likely go for good testing (as you have done), and then we can do a prerelease of the action itself which signifies to community that it's not production ready (and so they can test and also give feedback).

The other thing is that I want to bump to eventually (maybe with all these changes) a major version like v2.x instead of the current v0.2.1 since the Action has indeed been used by thousands of repos now from our insights and the semver would be easier understood with a real number against MAJOR.MINOR.PATCH

Thoughts?

- Create tests for the action pertaining to deleting tags and releases. This introduces Jest as a test runner and makes a few changes to make testing easier, including: - Explicitly using process.exit instead of returns since Jest doesn't allow top-level returns and process.exit can be mocked. - Splitting out the code that runs the action from the code the contains the action code. This allows us to run the action in our test directly and wait for the action run to finish by awaiting the promise it returns instead of just requiring the file. - Creating utility functions for creating the paths that are used to hit the GitHub API. This allows us to make assertions that fetch is called with the correct path and data while having to do as little string matching as possible, since that tends to be brittle. - Creating a "mock" fetch. This will be automatically used by Jest when we use jest.mock('./fetch.js') in a test since it's in the __mocks__ directory. This keeps us from making actual network calls when running tests and lets us test things like how the action behaves when it gets errors.

- The package-lock.json file tells NPM (and other package managers that support it) exactly what version of NPM packages are in use by a project. By including it into the repo, we can make sure that all contributors are using the exact same version of dependencies.

- This adds a GitHub Action that will run on every new PR that is pointed towards the master branch to run the test suite. Tests will run against Node 16 and Node 18.

wardellbagby · 2023-03-20T02:53:36Z

@thadguidry Alright, I think I've address everything now.

thadguidry · 2023-03-20T05:45:45Z

@thadguidry Alright, I think I've address everything now.

awesome! so I'll checkout and run npm run test

thadguidry · 2023-03-20T05:59:35Z

E:\GitHubRepos\delete-tag-and-release>npm run test

> [email protected] test
> jest index.spec.js

'jest' is not recognized as an internal or external command,
operable program or batch file.

E:\GitHubRepos\delete-tag-and-release>

wardellbagby · 2023-03-20T12:23:23Z

E:\GitHubRepos\delete-tag-and-release>npm run test

> [email protected] test
> jest index.spec.js

'jest' is not recognized as an internal or external command,
operable program or batch file.

E:\GitHubRepos\delete-tag-and-release>

@thadguidry since there's a new dependency added, you have to run npm install first. Apologies; I should've mentioned that.

thadguidry · 2023-03-21T00:39:46Z

@wardellbagby OK, the local testing passed.
Now, hmm, that is all mocked. Great! But what would be your ideas or plan to remain in sync with the GitHub API (since your mocking a portion of the API responses, yeah?) How do other Actions in the Marketplace do that if they also use mocking?

thadguidry · 2023-03-21T01:02:40Z

@constgen Do you happen to have insight on perhaps how the testing/mocking might get better also?

wardellbagby · 2023-03-21T01:20:23Z

Don't worry @thadguidry, I plan on following this PR up with one that ports it to using the Octokit libraries and that'll rewrite these tests as well.

I want this PR merged as the "safe" version that doesn't change any functionality so that the refactor will have a good tested base to compare to.

I'll also update the README as well with development instructions and implement Prettier and eslint for solid source code management.

wardellbagby commented Dec 16, 2022

View reviewed changes

wardellbagby force-pushed the wardell/add-tests branch 2 times, most recently from 26c89f5 to 663a16c Compare December 17, 2022 05:54

thadguidry reviewed Mar 20, 2023

View reviewed changes

wardellbagby force-pushed the wardell/add-tests branch 2 times, most recently from a323d92 to f3b7b2a Compare March 20, 2023 02:49

wardellbagby added 3 commits March 19, 2023 19:50

Add GitHub Action to run tests

cd5e6ec

- This adds a GitHub Action that will run on every new PR that is pointed towards the master branch to run the test suite. Tests will run against Node 16 and Node 18.

wardellbagby force-pushed the wardell/add-tests branch from f3b7b2a to cd5e6ec Compare March 20, 2023 02:50

thadguidry mentioned this pull request Mar 21, 2023

Use GitHub Actions SDK #22

Closed

thadguidry merged commit 6a81278 into dev-drprasad:master Mar 21, 2023

Jvr2022 mentioned this pull request Apr 4, 2024

chore: update deps wikimedia/apps-android-wikipedia#4580

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tests and CI #17

Add Tests and CI #17

wardellbagby commented Dec 16, 2022

wardellbagby Dec 16, 2022

thadguidry Mar 20, 2023

wardellbagby Mar 20, 2023

wardellbagby Mar 20, 2023

wardellbagby Dec 16, 2022 •

edited

Loading

thadguidry Mar 20, 2023

wardellbagby Mar 20, 2023

thadguidry Mar 20, 2023

thadguidry Mar 20, 2023

thadguidry commented Mar 20, 2023

wardellbagby commented Mar 20, 2023

thadguidry Mar 20, 2023

wardellbagby commented Mar 20, 2023

thadguidry commented Mar 20, 2023

thadguidry commented Mar 20, 2023

wardellbagby commented Mar 20, 2023 •

edited

Loading

thadguidry commented Mar 21, 2023

thadguidry commented Mar 21, 2023

wardellbagby commented Mar 21, 2023

Add Tests and CI #17

Add Tests and CI #17

Conversation

wardellbagby commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardellbagby Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thadguidry commented Mar 20, 2023

wardellbagby commented Mar 20, 2023

Choose a reason for hiding this comment

wardellbagby commented Mar 20, 2023

thadguidry commented Mar 20, 2023

thadguidry commented Mar 20, 2023

wardellbagby commented Mar 20, 2023 • edited Loading

thadguidry commented Mar 21, 2023

thadguidry commented Mar 21, 2023

wardellbagby commented Mar 21, 2023

wardellbagby Dec 16, 2022 •

edited

Loading

wardellbagby commented Mar 20, 2023 •

edited

Loading