chore(test): (almost) all up to sheet level #334
Conversation
bakbakbakbakbak
changed the title
bakbakbakbakbak
changed the title
|
Before I dive into a review (probably tomorrow night), this mass-renaming of top-level files exposes for me that our naming convention, in general, isn't great. thoughts? edit: actually this is perhaps a discord discussion. moving there |
| export default ({ describe, it, before, after, expect }: QuenchMethods) => { | ||
| describe("applyChatCardDamage(roll, multiplier)", () => {}); | ||
| describe("addChatMessageContextOptions(_, options)", () => {}); | ||
| describe("addChatMessageButtons(msg. html)", () => {}); | ||
| }; |
There was a problem hiding this comment.
Yes, and I did forget to make todo-comments for them, will update!
| it("Natural 1 total always fails", () => { | ||
| const roll = createMockRoll(1); | ||
| expect(OseDice.attackIsSuccess(roll, 10, 10)).equal(false); | ||
| }); | ||
| it("Natural 1 terms always fails", () => { | ||
| const roll = createMockRoll(5, [1]); | ||
| expect(OseDice.attackIsSuccess(roll, 10, 10)).equal(false); | ||
| }); | ||
| it("Natural 20 total always succeed", () => { | ||
| const roll = createMockRoll(20); | ||
| expect(OseDice.attackIsSuccess(roll, 100, 10)).equal(true); | ||
| }); | ||
| it("Natural 20+ total always succeed", () => { | ||
| const roll = createMockRoll(21); | ||
| expect(OseDice.attackIsSuccess(roll, 100, 10)).equal(true); | ||
| }); | ||
| it("Natural 20 terms always succeed", () => { | ||
| const roll = createMockRoll(5, [20]); | ||
| expect(OseDice.attackIsSuccess(roll, 100, 10)).equal(true); |
There was a problem hiding this comment.
As mentioned in Discord and logged in #340, please do something like
createMockRoll(1,[1])
expect(OseDice.attackIsSuccess(roll, 21, 0)).equal(false);
createMockRoll(1,[20])
expect(OseDice.attackIsSuccess(roll, 0, 21)).equal(true);
createMockRoll(20,[1])
expect(OseDice.attackIsSuccess(roll, 21, 0)).equal(false);
createMockRoll(20,[20])
expect(OseDice.attackIsSuccess(roll, 0, 21)).equal(true);(I'm only 50% sure those are the values we need in attackIsSuccess)
There was a problem hiding this comment.
Updated the tests with only these for testing natural 1 and 20s!
| it("Natural 1 terms is unsuccesful", async () => { | ||
| await game.settings.set(game.system.id, "ascendingAC", true); | ||
| expect(game.settings.get(game.system.id, "ascendingAC")).equal(true); | ||
| const roll = createMockRoll(1); | ||
| expect(OseDice.digestAttackResult(data, roll).isSuccess).equal(false); | ||
| expect(OseDice.digestAttackResult(data, roll).isFailure).equal(true); | ||
| }); |
There was a problem hiding this comment.
Need another createMockRoll(20, [1]) here
There was a problem hiding this comment.
Fixed, added both the createMockRoll(1, [1]) and createMockRoll(20, [1]).
There was a problem hiding this comment.
Ok yeah I agree that both is good, thanks!
| it("Natural 20 is succesful", () => { | ||
| const roll = createMockRoll(20); | ||
| expect(OseDice.digestAttackResult(data, roll).isSuccess).equal(true); | ||
| expect(OseDice.digestAttackResult(data, roll).isFailure).equal(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Need another createMockRoll(1, [20]) here
There was a problem hiding this comment.
Fixed, see previous comment on the Natural 1.
| it("Natural 1 terms is unsuccesful", async () => { | ||
| await game.settings.set(game.system.id, "ascendingAC", false); | ||
| expect(game.settings.get(game.system.id, "ascendingAC")).equal(false); | ||
| const roll = createMockRoll(1); | ||
| expect(OseDice.digestAttackResult(data, roll).isSuccess).equal(false); | ||
| expect(OseDice.digestAttackResult(data, roll).isFailure).equal(true); | ||
| }); |
There was a problem hiding this comment.
Need another createMockRoll(20, [1]) here
There was a problem hiding this comment.
Fixed, see above!
| it("Natural 20 is succesful", () => { | ||
| const roll = createMockRoll(20); | ||
| expect(OseDice.digestAttackResult(data, roll).isSuccess).equal(true); | ||
| expect(OseDice.digestAttackResult(data, roll).isFailure).equal(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Need another createMockRoll(1, [20]) here
There was a problem hiding this comment.
Fixed, see above.
|
When reviewing the missed @todo comments, I found out that I have already solved the issues with localization, so I've updated those in this latest commit as well.
|
I know I asked for smaller PRs, but since I got some kind of flow into this the last week, here we go.
I've rewritten most test suites I created. Also added test suites for almost all of the following:
I have also solved the issues encountered, but will make separate PRs for those. One for helper fixes, and one for the fixes that resolves the issues encountered.