-
Notifications
You must be signed in to change notification settings - Fork 57
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
chore(test): (almost) all up to sheet level #334
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I only have the one change requested if you can answer whether the others have been left empty intentionally
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the tests with only these for testing natural 1 and 20s!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found more changes in light of discovering #341
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need another createMockRoll(20, [1])
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, added both the createMockRoll(1, [1])
and createMockRoll(20, [1])
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need another createMockRoll(1, [20])
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need another createMockRoll(20, [1])
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need another createMockRoll(1, [20])
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.