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

✨ Add option to use original commit message #96

Merged
merged 2 commits into from
Sep 4, 2021
Merged

✨ Add option to use original commit message #96

merged 2 commits into from
Sep 4, 2021

Conversation

Nef10
Copy link
Contributor

@Nef10 Nef10 commented Sep 4, 2021

There are a lot of edge cases to consider for this feature, and I hope I covered all of them. The following are the once I tested:

With COMMIT_EACH_FILE set to true:

Test Original Message used Commit(s) Action Run(s) opened PR
Simple file change Link Link Link
Remove file Link Link Link
Add file Link Link Link
Change to sync config Link Link Link
Change of file with different destination name Link Link Link
Change of file in syncronized folder Link Link Link
Change of multiple files in syncronized folder Link Link Link
Change of files in different syncronized groups ✅ / ✅ Link Link Link
Change file together with non-syncronized file Link Link Link
Triggered by manually running the workflow In target repo Link Link
Push two commits up at once, changing same file 1 2 Link Link
Push two commits up at once, changing different file 1 2 Link Link
Previous PR not merged different file ❌ / ✅ 1 2 1 2 Link
Previous PR not merged same file 1 2 1 2 Link

With COMMIT_EACH_FILE set to false:

Test Original Message used Commit(s) Action Run(s) opened PR
Simple file change Link Link Link
Remove file Link Link Link
Add file Link Link Link
Change to sync config Link Link Link
Change of file with different destination name Link Link Link
Change of file in syncronized folder Link Link Link
Change of multiple files in syncronized folder Link Link Link
Change of files in different syncronized groups Link Link Link
Change file together with non-syncronized file Link Link Link
Triggered by manually running the workflow In target repo Link Link
Push two commits up at once, changing same file 1 2 Link Link
Push two commits up at once, changing different file 1 2 Link Link
Previous PR not merged different file 1 2 1 2 Link
Previous PR not merged same file 1 2 1 2 Link

@BetaHuhn
Copy link
Owner

BetaHuhn commented Sep 4, 2021

Awesome, thanks for this great PR! Really appreciate the amount of work you put into this, especially for the testing!

Will run a few tests myself and release it asap.

@BetaHuhn BetaHuhn merged commit c03c7b0 into BetaHuhn:develop Sep 4, 2021
@BetaHuhn
Copy link
Owner

BetaHuhn commented Sep 4, 2021

Manually creating/running all those tests for each change we introduce takes a lot of time, maybe we need to think about automating it with some kind of unit tests in the future. Probably not so easy to test as other actions since we are not only interacting with the GitHub API but also performing git operations.

What do you think?

@BetaHuhnBot
Copy link
Collaborator

🎉 This PR is included in version 1.12.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@Nef10
Copy link
Contributor Author

Nef10 commented Sep 4, 2021

Yes, definitely agree - when I started I did not think that there were this many cases to consider, otherwise I would have probably automated something from the beginning.

There are two parts: We should have some unit tests for the functions e.g. parseGitDiffOutput should be simple to test. As you did not have any tests yet, I did not wanted to go go ahead and set everything up (testing framework, changes to action workflows, ...)

But the bigger part is probably having proper integration tests. It might be some work but it roughly could work like this:

  1. You have two dedicated repos, just like I used for my manual tests.
  2. The tests would need access with a GitHub app to interact with them. (As a the default GitHub token would not trigger workflows)
  3. Create a dedicated branch on the second repo to run tests agains
  4. Than you can create a branch on the first repo and configure it to use the action version from the PR branch, targeting the new branch from above
  5. You check out the repo
  6. Do the changes (like I did) and push them up
  7. Wait for the PR to be created, check the message (and maybe content?) via the GitHub API
  8. Merge PR and repeat from step 6

With this setup you can probably tests all feature of this action.

@Nef10 Nef10 deleted the original-message branch September 5, 2021 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants