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

change expected damage to make the tests pass #67

Closed
wants to merge 2 commits into from
Closed

change expected damage to make the tests pass #67

wants to merge 2 commits into from

Conversation

DustinAlandzes
Copy link
Contributor

@DustinAlandzes DustinAlandzes commented May 9, 2021

the tests in master are failing right now, it's two tests in the same test suite

when calculating stats › when calculating total damage done › should ignore Pichu's self-damage
when calculating stats › when calculating total damage done › should include pummel damage

both are in test/stats.spec.ts

I changed these numbers:
21 -> 0
32 -> 22

https://github.com/project-slippi/slippi-js/pull/66/checks?check_run_id=2539370690#step:6:368
image

image

I think these commits caused the test to fail after changing the way damage was calculated
e61a58a
02ba277

image

@DustinAlandzes DustinAlandzes changed the title change expected damage values to make the tests pass change expected damage to make the tests pass May 9, 2021
@vinceau
Copy link
Member

vinceau commented May 10, 2021

The tests in their current state are correct. You shouldn't make the tests incorrect just to make the tests pass. That defeats the purpose of the tests. For now, we've reverted the double stats computation logic (the cause of the failing tests) in #71.

@vinceau vinceau closed this May 10, 2021
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.

None yet

2 participants