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

A fix for a bug with the -H option. It did not show anything when a period with no transactions was chosen rather than show the ending balances. #583

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@nniro

nniro commented Jul 6, 2017

These set of patches add a testing context for the fix and the fix itself.
The testing context is to make sure the fix does in fact behave like it should. They were added for BalanceReport and MultiBalanceReport only.

this is an excerp of one of the patches description :

This patch fixes a bug that happened when using the -H option on
a period without any transaction. Previously, the behavior was no output
at all even though it should have shown the previous ending balances of
past transactions. (This is similar to previously using -H with -E, but
with the extra advantage of not showing empty accounts)

@nniro

This comment has been minimized.

Show comment
Hide comment
@nniro

nniro Jul 6, 2017

Ah I didn't notice the warnings that samplejournal2 is already initialized elsewhere in the project. I'll need someone to tell me where to put the new samplejournal (or just rename it to samplejournal3). At first, I wanted to update samplejournal with my new entry and then change all the tests that failed to reflect the new journal, but I wasn't sure if that would be accepted or what.

nniro commented Jul 6, 2017

Ah I didn't notice the warnings that samplejournal2 is already initialized elsewhere in the project. I'll need someone to tell me where to put the new samplejournal (or just rename it to samplejournal3). At first, I wanted to update samplejournal with my new entry and then change all the tests that failed to reflect the new journal, but I wasn't sure if that would be accepted or what.

@nniro nniro changed the title from the tests and the fix for the -H bug to A fix for a bug with the -H option. It did not show anything when a period with no transactions was chosen rather than show the ending balances. Jul 7, 2017

Nicholas Niro added some commits Jul 6, 2017

Nicholas Niro
lib: Added 2 new tests to BalanceReport.
These tests verify the behavior when we input a very specific period.
The second test is meant to make sure that the new upcoming code in
MultiBalanceReports will not change anything in the behavior of
the module BalanceReport.
Nicholas Niro
lib: Implemented a testing context for the module MultiBalanceReports.
Of the 2 tests, the first is a simple test on a specific period.
The second is expected to fail at this point until the new upcoming
code to fix the issue with the history option is implemented.

For the record : this issue happens when we use the -H flag for a period
that does not contain any transactions. Currently, the ending balance
values are only taken into account if the current period contains
a Transaction containing one of the previous populated accounts.

For example, if we have a statement on the 2008/01/01 for $1
and we do a command (with -H) to check the value on the
(without transactions) 2008/01/02, we will not get the $1 from
2008/01/01. In that same example, if we had a transaction for the same
account as 2008/01/01 in say 2008/01/03 then the -H command would
successfully show the statement from 2008/01/03 with the initial amount
that we set in 2008/01/01.
Nicholas Niro
lib: Added a new transaction to samplejournal.
The new entry effectively adds a loan which is placed in the checking account.
This loan is then closed by the "pay off" transaction (which was already
present).

This is mainly to be used as a test point for the -H option; to make
sure -H does not show empty accounts.

All previous tests were changed to reflect the new change.
The documentation of the journal module was updated too.
Nicholas Niro
lib: Added a new much more thorough test to MultiBalanceReports.
This test makes sure that -H works correctly and it does not show
empty accounts.
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jul 11, 2017

Owner

Status check ? I think you're still working on this and I don't need to do anything at the moment.

Owner

simonmichael commented Jul 11, 2017

Status check ? I think you're still working on this and I don't need to do anything at the moment.

Nicholas Niro
lib: Fix of a bug with the -H option.
This patch fixes a bug that happened when using the -H option on
a period without any transaction. Previously, the behavior was no
output at all even though it should have shown the previous ending balances
of past transactions. (This is similar to previously using -H with -E,
but with the extra advantage of not showing empty accounts)
@nniro

This comment has been minimized.

Show comment
Hide comment
@nniro

nniro Jul 11, 2017

Finally got travis happy.

nniro commented Jul 11, 2017

Finally got travis happy.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jul 15, 2017

Owner

I see examples/sample.journal hasn't been changed. We have both samplejournal and sample.journal because unit tests needed a pure value, functional tests needed a file. Until now they have been the same, for simplicity and ease of moving tests between unit and functional. I would probably keep it that way, unless you think it's fine to let them diverge ? If so maybe we should rename one of them, samplejournal -> testjournal or something.

Re unit tests vs functional tests: I did find it quite hard to understand/replicate/test this after being away from it; functional tests would have helped me process it more quickly. But unit tests run faster and are more focussed, and we should probably try them some more, so I'm fine with them.

Owner

simonmichael commented Jul 15, 2017

I see examples/sample.journal hasn't been changed. We have both samplejournal and sample.journal because unit tests needed a pure value, functional tests needed a file. Until now they have been the same, for simplicity and ease of moving tests between unit and functional. I would probably keep it that way, unless you think it's fine to let them diverge ? If so maybe we should rename one of them, samplejournal -> testjournal or something.

Re unit tests vs functional tests: I did find it quite hard to understand/replicate/test this after being away from it; functional tests would have helped me process it more quickly. But unit tests run faster and are more focussed, and we should probably try them some more, so I'm fine with them.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jul 15, 2017

Owner

I have rebased and merged. Thanks for the fix, and good catch! I wonder how long it has been undetected.

Owner

simonmichael commented Jul 15, 2017

I have rebased and merged. Thanks for the fix, and good catch! I wonder how long it has been undetected.

simonmichael added a commit that referenced this pull request Aug 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment