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

TR unit tests #270

Closed
wants to merge 11 commits into from
Closed

Conversation

christopherlam
Copy link
Contributor

Work in progress. Divided renderer into multistage, will now write tests.

@christopherlam christopherlam force-pushed the TR-unittest branch 12 times, most recently from ee74d24 to 8402dd7 Compare February 4, 2018 06:56
@christopherlam christopherlam force-pushed the TR-unittest branch 4 times, most recently from 0beb469 to fa32cc3 Compare February 11, 2018 12:21
@christopherlam
Copy link
Contributor Author

christopherlam commented Feb 11, 2018

So far the unit tests are rather simplistic and tests mainly simple filtering and display elements. Genuine testing of sortorders, currency conversions etc is far more difficult and requires html parsing :-(

@christopherlam christopherlam force-pushed the TR-unittest branch 7 times, most recently from a0eaebe to bc30fa8 Compare February 18, 2018 02:21
@christopherlam christopherlam force-pushed the TR-unittest branch 4 times, most recently from 3345c7c to 24b4bf7 Compare February 20, 2018 01:45
@jralls
Copy link
Member

jralls commented Feb 20, 2018

Could you be a bit more expressive with the commit messages?

@christopherlam
Copy link
Contributor Author

Could you be a bit more expressive with the commit messages?

Hmm I'm still trying to find the right toolchain, and I've rebase/fixup for now. Unfortunately travis-ci guile uses guile-2.0.9 whch doesn't have srfi-64 :(

@christopherlam
Copy link
Contributor Author

For travis to pass I may have to forego srfi-64 and write custom macros instead, which are not that difficult.

@jralls
Copy link
Member

jralls commented Feb 20, 2018

I'm not sure what Travis's limitations have to do with cryptic commit messages, but if the purpose of the PR is to run Travis on work that isn't actually ready to merge, there are a few alternatives that might work a bit better:

  • You can turn on Travis for your own Github repository.
  • You can set up docker and run the tests in the same docker environments that Travis uses, but on your own machine
  • You can set up some other sort of VM (VirtualBox is popular for Linux) with OS images that match the Travis setup. This might be the best solution for your purpose because it allows full interaction with the VM without having to set up VNC connections by hand.

@christopherlam
Copy link
Contributor Author

Wish to canvas opinion about a hack to add srfi-64 in travis -- I've copied srfi-64 into common/test-core/ and modified commonbuild to check for srfi-64, and copy it if does not exist (ie guile-2.0.9 or earlier). This passes on travis. It'll be a shame to seek another unit-test toolchain when srfi-64 is very well designed. Will this hack be acceptable? See latest commit on https://github.com/christopherlam/gnucash/commits/TR-unittest

@christopherlam
Copy link
Contributor Author

christopherlam commented Feb 21, 2018

Code that's lifted from somewhere else goes into "borrowed".
But I think it would be simpler and better to test for SRFI-64 in the root CMakeLists.txt and enable your unit tests only if it's available. Geert, what's your opinion?

Whoops. Rebasing meant the 6a173f6 commit including comment was lost. I've moved to ./borrowed in my tree. I was fighting with travis all day until this worked. SRFI-64, SXML and SXPATH is a good combination for stress testing reports. With this I can be much more productive.

@gjanssens
Copy link
Member

I would definitely prefer testing for SRFI-64 in the root CMakeLists.txt and only enable scheme unit testing when it's available.
Note also the way you added your test in cicommon in the 6a173f6 is not very portable. It will fail on a system which only has guile 2.2 installed. You could add an extra test for guile 2.2 but it quickly gets confusing. And it would break again when guile releases 2.4.
Better would be to run guile -c '(use-modules (srfi srfi-64))' and check whether that fails or not.

@christopherlam
Copy link
Contributor Author

christopherlam commented Feb 21, 2018

I would definitely prefer testing for SRFI-64 in the root CMakeLists.txt and only enable scheme unit testing when it's available.

Ok - this would mean travis-arch has guile-2.2 can run the tests and travis-ubuntu with guile-2.0.9 will skip them. A local build with guile 2.0.10 onwards will run the tests successfully.

Note also the way you added your test in cicommon in the 6a173f6 is not very portable. It will fail on a system which only has guile 2.2 installed. You could add an extra test for guile 2.2 but it quickly gets confusing. And it would break again when guile releases 2.4.
Better would be to run guile -c '(use-modules (srfi srfi-64))' and check whether that fails or not.

The reasoning was, given guile>=2.0.10 always has srfi-64, we'd only need to test that the /usr/share/guile/2.0/ path exists and doesn't contain srfi-64. guile-2.2 onwards would skip the whole 'if' section.

Alternatively the following?

 if ! guile -c '(use-modules (srfi srfi-64))'; then
    # copy srfi into /usr/share/guile/2.0/ <-- still hardcoded to guile-2.0
 fi

@christopherlam
Copy link
Contributor Author

PS it's still preferable to disable tests for travis-ubuntu then please let me know how CMakeLists.txt should change!

@jralls
Copy link
Member

jralls commented Feb 22, 2018

Run the test Geert proposed above and set a variable HAVE_SRFI64 based on the results. Then in the various test/CMakeLists.txt call add_scheme_test for srfi64 based tests only if HAVE_SRFI64 is true.

@christopherlam
Copy link
Contributor Author

Ok - the following does the trick - will enable HAVE_SRFI tests if guile >= 2.0.10 - the downside is an ugly error message during build script. Alternatively I could also add to guile22 section to quietly enable HAVE_SRFI64.

# Test that guile has SRFI-64. This is required for some unit tests.
EXECUTE_PROCESS( COMMAND ${GUILE_EXECUTABLE} -c "(use-modules (srfi srfi-64))"
  RESULT_VARIABLE GNC_SRFI64_RESULT
)

 IF (GNC_SRFI64_RESULT EQUAL 0)
  MESSAGE(STATUS "Using guile SRFI-64")
  SET(HAVE_SRFI64 TRUE)
ENDIF()

@jralls
Copy link
Member

jralls commented Feb 22, 2018

You can probably eat the error message with

EXECUTE_PROCESS( COMMAND ${GUILE_EXECUTABLE} -c "(use-modules (srfi srfi-64))" > /dev/null 27>1

@gjanssens
Copy link
Member

Or more the cmake way by adding "ERROR_QUIET" to the execute_process invocation. Optionally, it may also need "OUTPUT QUIET" if guile is not writing to stderr.

And please write cmake commands in lowercase. I know most of it is upper case, but I want to fix this for 4.0. By writing lowercase now for new cmake functionality, there's less to change in the next dev cycle.

@gjanssens
Copy link
Member

Hmm, looks like you figured out the "ERROR_QUIET" option already in PR286. Only the lowercase request remains then.

This was referenced Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants