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

[Alpheus]

Linked issues

tests #1693

Bug fix

Reported by catlover as we expanded the cap on Bladeburner skills to up to Number.MAX_VALUE.
This draft PR provides coverage for the formulas responses that seem faulty. This is currently in dev and hasn't been released to 2.6.3 yet.

[catloversg]

Can you change the title to something indicating that this PR adds tests for skillMaxUpgradeCount? Having the same title as the issue is confusing.

[d0sboots]

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(

[Alpheus]

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

[d0sboots]

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

[Alpheus]

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.

[d0sboots]

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.

[Alpheus]

Update: I managed to capture the bug better and narrow it to an MRE with regards to cost.
The fix still eludes me.

[Alpheus]

@d0sboots @catloversg Requesting advice. I'm close, here's my context dump:

• we unlocked upgrading skills past MAX_SAFE_NUMBER in bcb4a38
• this introduces the scenario where current level + upgrades = current level due to floating point errors: bcb4a38#diff-4d95b76d4df7dedcb50e350d2afb8901d87780532239de973e1e0db6deded91bR154
• this case is handled on upgradeSkill (but not upgradeCost) introduced the fix for issue#1071
• the BB Skill UI makes the it appear as if cost is zero and the purchase buttons stop working
• you need to purchase a certain amount of skill to get past the floating rounding error.

Here's my breakdown, hopefully I got the severity right. Let me know:
Definitely an issue

• "Skill points required" is shown to be 0 on UI past this threshold, but requires an explicit CLICK to get past react state memoization, seems broken
• The PLUS sign on skills UI does nothing when in this state (not even error msg)
• The message "Cannot upgrade ${skillName}: Due to floating-point inaccuracy and the small value of specified "count", your skill cannot be upgraded." is incorrect. The player should be instructed to upgrade more levels in bulk. This message is shown primarily in the BB terminal when using "skill level [skill name]"

Possibly an issue

• the Skill.calculateCost method that can be accessed through the NS API ns.bladeburner.getSkillUpgradeCost returns 0 when in this state for counts below the threshold. This seems to be a breaking change as players have been using this function to do a binary search to find the upper threshold for cost (ie stop the search).
• the introduction of Skill.skillMaxUpgradeCount method that can be accessed through the NS API ns.formulas.bladeburner.skillMaxUpgradeCount may be too subtle to find for the player to see that this is a solveable problem without brute-forcing.

Nice to have (for int farmers)

• Have the PLUS purchase button on BB UI purchase maximum points or add a second button to do that
• Add an option to BB terminal to specify upgrade count (or MAX)
• Change text on BB Skill UI to show cost of max purchase
• When cost is zero, but upgradable, show the floating-point explanation in a tooltip on UI.

---

I should mention that prior to this release (in 2.6.2), the getSkillUpgradeCost returning an actual number made it easier to do a binary search prior to discovering the BB formulas.

[catloversg]

As requested, I'll copy my response in Discord to here.

Int farming
I'll be clear about this "unique gameplay": It's unsupported. There are some bugs/oversights that lead to interesting gameplay. Generally, if they don't affect normal gameplay and their respective gameplay has enough "support", we will turn a blind eye. With these kinds of gameplay, we won't actively try to kill them, but we also won't change the UI/API for them. In a normal BB run, there is no way that skill level and skill cost are bigger than Number.MAX_SAFE_INTEGER. The only reason that we have to deal with this "big number" issue is that we still allow int farming. I'll try to provide reasonable support for logic-related things (e.g., proper internal calculation in calculateCost, calculateMaxUpgradeCount, etc.). For all other things (e.g., UI), it's "out-of-scope".

calculateCost returns 0 when count is too small
This is the correct behavior. In old versions, it returns a value, but that value may be misleading.

/** @param {NS} ns */
export async function main(ns) {
  Player.bladeburner.skillPoints = 1e300;
  Player.bladeburner.skills["Hyperdrive"] = 1e100;
  const before = ns.bladeburner.getSkillLevel("Hyperdrive");
  console.log(`Current level: ${before}`);
  console.log(`Required SP: ${ns.bladeburner.getSkillUpgradeCost("Hyperdrive")}`);
  ns.bladeburner.upgradeSkill("Hyperdrive");
  const after = ns.bladeburner.getSkillLevel("Hyperdrive");
  console.log(`Current level: ${after}`);
  if (before === after) {
    console.log("No change");
  }
}

In the example above, getSkillUpgradeCost returns 2.5e+100 (tested on v2.6.0). A naive player may think that as long as they have more than 2.5e+100 SP, they can call ns.bladeburner.upgradeSkill("Hyperdrive") and their Hyperdrive's level will be increased.

I agree that "returning 0" is not ideal, but at least it indicates that something went wrong. I'll clarify this behavior and mention the new formula API in the TSDoc. If the player wants to implement their "binary search", they can still do that. Maybe they have to change it, though.

The message "Cannot upgrade ..." is incorrect.

That error message is correct. It explained exactly what happened. With BB terminal, the player can only upgrade a skill by 1 level. When "+1 level" is not enough to "overcome" the floating-point inaccuracy, this error message is shown.

For all other UI things, it's out-of-scope.

[d0sboots]

Also per long discord discussion, copied (but not verbatim) here:

• Agreed that calculateCost should return 0 in the scenarios we're discussing.
• canUpgrade should mirror the current behavior of upgradeSkill, so that it returns an error instead of {true, 0} for the case we're discussing.
• The UI should be disabled above MAX_SAFE_INTEGER, so that it doesn't flicker or have buttons that don't work. An alternative would be plumbing calculateMaxUpgradeCount to the UI so that the UI becomes more generally useful, in this situation and regular play. But it still shouldn't flicker.

[Alpheus]

Also per long discord discussion, copied (but not verbatim) here:

• Agreed that calculateCost should return 0 in the scenarios we're discussing.
• canUpgrade should mirror the current behavior of upgradeSkill, so that it returns an error instead of {true, 0} for the case we're discussing.
• The UI should be disabled above MAX_SAFE_INTEGER, so that it doesn't flicker or have buttons that don't work. An alternative would be plumbing calculateMaxUpgradeCount to the UI so that the UI becomes more generally useful, in this situation and regular play. But it still shouldn't flicker.

Perfect! I'll be traveling starting this week so will pick up hopefully end of november.

[d0sboots]

Perfect! I'll be traveling starting this week so will pick up hopefully end of november.

Have fun! We know I am not super speedy anyway XD