Skip to content

Conversation

@nniro
Copy link

@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
Copy link
Author

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 the tests and the fix for the -H bug 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
nniro added 5 commits July 10, 2017 11:38
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.
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.
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.
This test makes sure that -H works correctly and it does not show
empty accounts.
@simonmichael
Copy link
Owner

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

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
Copy link
Author

nniro commented Jul 11, 2017

Finally got travis happy.

@simonmichael
Copy link
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
Copy link
Owner

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants