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

API: Add tests for resolving issues with 'calculateCost' on dev (targeted 2.6.3) #1724

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions test/jest/Netscript/Bladeburner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { BladeburnerSkillName } from "../../../src/Enums";
import { PositiveInteger, isPositiveInteger, isPositiveNumber } from "../../../src/types";
import { randomInRange } from "../../../src/utils/helpers/randomInRange";
import { getRandomIntInclusive } from "../../../src/utils/helpers/getRandomIntInclusive";
import { Skills } from "../../../src/Bladeburner/data/Skills";

const skill = new Skill({
name: BladeburnerSkillName.Hyperdrive,
Expand All @@ -14,6 +15,18 @@ const skill = new Skill({
});

describe("Test calculateMaxUpgradeCount", function () {
it.failing("Bug: Returns 1 when SP is too small and current level is too high", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please name these with the expected value of how they should work, not the current buggy state. That way when they are fixed they can just be flipped to it(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know I'm not actually sure what the correct behavior is (at least the one's we wish to fix according to the discussion in the issue).

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't know what the correct behavior is, then it's probably not the right time to be writing tests. XD

Copy link
Contributor Author

@Alpheus Alpheus Oct 29, 2024

Choose a reason for hiding this comment

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

I don't know how to write the test you suggested (the inverse case). The test I wrote covers the bug, not the fix.

Edit: I can naively claim that return value should be 1, though I do not know how to verify whether that is doable with the current formula.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you misunderstood what I was asking for. You don't need to be able to fix the bug, or even write a fully fleshed out "regular" test in a situation like this. However, you ought to be able to at least describe in words, for the test description, how the code "ought" to behave, if you think it actually has a bug. Even just inverting what you have ("Should not return 1 at high levels with small SP") would be sufficient, and explains the construction of your test.

expect(Skills.Hyperdrive.calculateMaxUpgradeCount(1e50, 1)).toBe(0);
});

it.failing("Bug: Returns 1 when SP is too small and current level is too high", () => {
expect(Skills.Hyperdrive.calculateMaxUpgradeCount(1e50, Number.MAX_SAFE_INTEGER)).toBe(0);
});

it.failing("Bug: Returns Infinity when SP is large enough and current level is too high", () => {
expect(Number.isFinite(Skills.Hyperdrive.calculateMaxUpgradeCount(1e50, Number.MAX_VALUE))).toBeTruthy();
});

test("errorCount", () => {
let testCaseCount = 0;
let errorCount = 0;
Expand Down