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

test: fix net qty and HU tests #117

Merged
merged 3 commits into from
May 13, 2024
Merged

test: fix net qty and HU tests #117

merged 3 commits into from
May 13, 2024

Conversation

HKuz
Copy link
Collaborator

@HKuz HKuz commented May 12, 2024

Addresses #98

  • Net quantity test (currently skipped) - validate underlying behavior
  • Refactor two xfail tests to use context manager with expected error
  • Add pytest-order dependency before we need it

After rebasing with staging, 4 other tests failed - this uncovered a few areas we needed to adjust either the test or add checks in the code.

  • check if an item has enable_handling_unit checked (as well as is_stock_item)
  • check if a BOM scrap item has 'create a handling unit' checked

Additionally, there was a timing issue on the Stock Entry doc event in hooks. The validate_items_with_handling_unit in validate was throwing errors for missing handlings units when they weren't created yet (that step is done in before_submit via generate_handling_units). I moved the validate function to run AFTER the handling units are generated, now both in the before_submit hook

Net Quantity

For net quantity, the underlying Stock Ledger entries use the transfer_qty field (from the Stock Entry Detail item row) when updating the handling unit's stock_qty. This field gets set and updated in the UI when the qty field changes, so in UI tests to transfer part of an HU, the stock ledger correctly shows net totals. Good news, most of the issue here was tied to fields not being set programmatically in the tests (it's done on a field change in the JS, not server-side), so the main "fix" was to explicitly set a few more fields in the tests.

The failing test had the following issues:

  • the repack test was running first, which changed the Parchment Paper HU. The net qty test then would grab the 'old' HU with stock_qty of 0 and show a negative balance. Separately, the overconsumption code wasn't throwing an error when this happened because it was looking for the actual_qty field, which was zero (the field doesn't get set programmatically in the tests or on save), so there's no overconsumption when zero units are removed from the HU. I created a 'clean' Material Receipt for Parchment Paper and used its HU for the test to avoid the issue
  • The transfer_qty field (what the stock ledger entries use to adjust the HU's quantity) also wasn't being set on save/submit programmatically in the test, so the code was 'netting out' zero from the HU. This caused the Assertion Error when the test checks if the HU qty == 95 because the math did 100 - 0 instead of 100 - 5. The test now explicitly sets the transfer_qty field, so the calcs have the right value to work with

Overconsumption Updates (FOR DISCUSSION):

When the overconsumption code was turned on (uncommented in hooks.py, and tests not skipped), test_handling_units_overconsumption_in_material_transfer_stock_entry failed because it didn't raise the known error. The first issue was that the actual_qty field wasn't being set in the test, which is what the overconsumption check uses to compare to the HU stock_qty. The second issue was that the calc itself didn't catch when the test tried to use 8 units from a HU of 5 units.

Some changes:

  • In general, the overconsumption check wasn't catching things because of behavior associated with the actual_qty field it was using to compare against the HU's quantity. The actual_qty field in a Stock Entry Detail item row gets set when an item is added/source WH is set, but doesn't change when the qty field changes in the UI or when I hit Save. So if I scan an HU, then manually adjust the qty, the actual_qty field doesn't update (see screen shot). To avoid mismatched quantity issues and to conform the overconsumption code with how the Stock Ledger Entries calculate net quantity, I changed the calculation to use the transfer_qty field
  • I adjusted the calculation so the code throws an error when the Stock Entry tries to consume/use/transfer 8 units of a HU with 5 units in it. This change made the "#incoming" code block math the same as the "#transfer" one, so I combined them
  • For the "#outgoing" code block (when there's only a t_warehouse, like for finished goods or a Material Receipt), I had a little trouble writing a test because of the sequence of events in that scenario. HUs are generated for those rows in before_submit, so it's tricky to change the qty after that happens to trigger an error in the test. I was wondering why the math was different in this case - maybe this is too simplistic, but when I think of how to define overconsumption, I figure if the quantity we're receiving/issuing/transfering/etc (row qty) less the HU qty is more than precision threshold, then it's overconsumption / throw error

Screen shot shows the values for the key fields we're using - when I changed the qty field and re-ran the console, it shows the qty and transfer_qty fields update, the actual_qty field remains the value that was set once the WH was added for the item. I also checked after adding a HU and after hitting save - same result.

Screenshot 2024-05-12 at 3 14 09 PM

@HKuz HKuz linked an issue May 12, 2024 that may be closed by this pull request
3 tasks
Copy link
Owner

@agritheory agritheory left a comment

Choose a reason for hiding this comment

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

Well done. I knew it was going to be a combination of things that seemed subtle and not obvious.

I'd like to merge this as-is and we'll make the changes to the ordering when we start incorporating the tests for demand, which will renumber them all anyway.

@@ -19,6 +20,7 @@ def submit_all_purchase_receipts():
pr.submit()


@pytest.mark.order(1)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's increment these to start at 20 in case there are tests we want to run before these. We also want to add pytest-orer to the other two test files, skip at least 10 in between.

@agritheory agritheory merged commit e33d501 into staging May 13, 2024
4 checks passed
@agritheory agritheory mentioned this pull request May 13, 2024
@HKuz
Copy link
Collaborator Author

HKuz commented May 13, 2024

@agritheory sounds good - the ordering I put in here ended up not being necessary (I de-coupled the repack and net qty tests so they weren't interfering). We can safely change/redo the ordering when needed.

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.

Handling unit test cleanup
2 participants