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

Mineunit tests #4

Open
wants to merge 61 commits into
base: master
Choose a base branch
from
Open

Mineunit tests #4

wants to merge 61 commits into from

Conversation

S-S-X
Copy link
Member

@S-S-X S-S-X commented Dec 6, 2021

Basic tests also showing bit more proper fixture use mentioned at #2 (comment)

API is used in fixture, that's not exactly correct use and API call should be in spec instead but I guess you'll get the idea.

  • Wrench
    • bitchange:shop
      • can be picked up
      • correctly placed after picked up
    • default:cobble
      • cannot be picked up
    • no-op tool use
      • works pointing nothing

There's no coverage reporting or any comments added by bot if tests pass, failure comment is added with test logs if tests fail.

There's a lot of commits because this branch is currently based on wrench_support_extend
All stuff is really in this single commit: 3334803

Changed base branch to be wrench_support_extend, if merged this can however be merged directly into master after #2 is done. I do recommend that because #2 is huge mixed changes already, bit over sized PR already.

@github-actions

This comment has been minimized.

@S-S-X
Copy link
Member Author

S-S-X commented Dec 6, 2021

Well yeah, also this depends on unreleased Mineunit updates: S-S-X/mineunit#67
Will prob merge that soon too.
✔️ done

@S-S-X S-S-X changed the base branch from master to wrench_support_extend December 6, 2021 11:52
@OgelGames
Copy link
Contributor

Is this really needed? I think manual testing is enough for a single tool.

@S-S-X
Copy link
Member Author

S-S-X commented Dec 6, 2021

Is this really needed? I think manual testing is enough for a single tool.

No, automated testing of course is not needed. It is just to catch errors and skip a lot of manual testing.

@S-S-X S-S-X mentioned this pull request Dec 6, 2021
@BuckarooBanzay BuckarooBanzay added the enhancement New feature or request label Dec 8, 2021
@OgelGames
Copy link
Contributor

I had a closer look at this, and it only tests the core functionality of the wrench - something that isn't likely to change at all, especially not frequently enough to need automated tests.

@OgelGames OgelGames closed this Dec 8, 2021
@S-S-X
Copy link
Member Author

S-S-X commented Dec 8, 2021

I had a closer look at this, and it only tests the core functionality of the wrench - something that isn't likely to change at all, especially not frequently enough to need automated tests.

That sound very ignorant especially because things that are not supposed to change are exactly ones you do create automated tests.

@S-S-X
Copy link
Member Author

S-S-X commented Dec 8, 2021

I've reopened this because there was one approval already. It tells that one people does not like it while apparently at least 2, possibly 3 agrees (judging by reactions and approvals).

@S-S-X S-S-X reopened this Dec 8, 2021
@nixnoxus nixnoxus mentioned this pull request Jan 7, 2022
@SwissalpS
Copy link

I also think unit tests are good to confirm that core functionality has not changed with possibly irrelevant looking additions.
The additions should also get tests because next editor will not remember to test everything manually each time.

Sometimes unit tests seem irrelevant and redundant as one has just written the code and the test with same mindset in mind.
However, they can catch errors just as luacheck catches syntax errors or unused vars etc.

Base automatically changed from wrench_support_extend to master March 15, 2022 04:00
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.

5 participants