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

chore(test): (almost) all up to sheet level #334

Merged
merged 7 commits into from
Feb 10, 2023

Conversation

bakbakbakbakbak
Copy link
Collaborator

@bakbakbakbakbak bakbakbakbakbak commented Feb 5, 2023

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:

  • helpers
  • dialog
  • item (up to sheet level)
  • party (up to sheet level, incl xp)

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.

@bakbakbakbakbak bakbakbakbakbak changed the title Massive test-suite Chore(test): Massive test-suite Feb 5, 2023
@bakbakbakbakbak bakbakbakbakbak changed the title Chore(test): Massive test-suite Chore(test): (almost) all up to sheet level Feb 5, 2023
@bakbakbakbakbak bakbakbakbakbak changed the title Chore(test): (almost) all up to sheet level chore(test): (almost) all up to sheet level Feb 5, 2023
@anthonyronda anthonyronda self-requested a review February 6, 2023 20:30
@anthonyronda
Copy link
Member

anthonyronda commented Feb 6, 2023

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

Copy link
Member

@anthonyronda anthonyronda left a 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

Comment on lines 19 to 23
export default ({ describe, it, before, after, expect }: QuenchMethods) => {
describe("applyChatCardDamage(roll, multiplier)", () => {});
describe("addChatMessageContextOptions(_, options)", () => {});
describe("addChatMessageButtons(msg. html)", () => {});
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these skipped?

Copy link
Collaborator Author

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!

Comment on lines 215 to 233
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);
Copy link
Member

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)

Copy link
Collaborator Author

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!

Copy link
Member

@anthonyronda anthonyronda left a 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

Comment on lines 269 to 275
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);
});
Copy link
Member

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

Copy link
Collaborator Author

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]).

Copy link
Member

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!

Comment on lines 291 to 296
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);
});
});
Copy link
Member

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

Copy link
Collaborator Author

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.

Comment on lines 298 to 304
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);
});
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, see above!

Comment on lines 320 to 325
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);
});
});
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, see above.

@bakbakbakbakbak
Copy link
Collaborator Author

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.

  • data-model-item-weapon.test.ts
  • data-model-item-spell.test.ts
  • data-model-item-ability.test.ts

Copy link
Member

@anthonyronda anthonyronda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@anthonyronda anthonyronda merged commit f8ea7b8 into vttred:1.8.x Feb 10, 2023
@bakbakbakbakbak bakbakbakbakbak deleted the chore/test_it_all branch February 10, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants