Automated testing for the new REMIND Library

[Loisel]

The test tries to create a reporting for a GDX from here: /p/projects/remind/users/renatoro/REMIND-EU_Calibration/v12_2021_02_10_mergeVersion/output/SSP2-Base_2021-02-10_15.40.32/fulldata.gdx. The GDX is hosted by RSE and downloaded from there for the test.

• If a GDX is placed in tests/testgdxs, no download takes place in the provided GDX is used.
• README is updated.
• A template is provided for summation tests on the reporting output.

A test on correct summation of |+| variables is in preparation by @giannou .

[Renato-Rodrigues]

@giannou If you want a quick and dirty code that does the summation test just let me know and I will send you my test script for that.

@Loisel, did you implemented an option to disable the gdx testing if someone just wants to test the build library instead of commiting directly the results?

[giannou]

@giannou If you want a quick and dirty code that does the summation test just let me know and I will send you my test script for that.

sure

[Loisel]

@giannou If you want a quick and dirty code that does the summation test just let me know and I will send you my test script for that.

@Loisel, did you implemented an option to disable the gdx testing if someone just wants to test the build library instead of commiting directly the results?

There is now a line at the beginning of the test that can be uncommented to skip the test.

[Renato-Rodrigues]

@Loisel I am getting this error while trying to build the library:

`

== Failed tests ================================================================
-- Failure (test-convGDX2mif.R:46:5): Test if REMIND reporting is produced as it should and check data integrity --
md5sum(file.path(testgdx_folder, "fulldata.gdx")) not equal to "8ce7127ec26cb8bb36cecee3f4fa97f1".
names for target but not for current
-- Error (test-convGDX2mif.R:103:3): Test if REMIND reporting is produced as it should and check data integrity --
Error: Could not read GDX file ../testgdxs/fulldata.gdx: Expected data marker (SYMBOL_1) not found in GDX file (rc=0)

Backtrace:
x

1. -remind2:::runParallelTests(my_gdxs) test-convGDX2mif.R:103:2
2. -remind2::convGDX2MIF(i) test-convGDX2mif.R:95:8

3. \-remind2::toolRegionSubsets(gdx)
   

4.   \-gdx::readGDX(gdx, name = "regi2iso")
   

5.     \-gdxrrw::gdxInfo(path.expand(gdx), dump = FALSE, returnDF = TRUE)
   

[ FAIL 2 | WARN 0 | SKIP 0 | PASS 0 ]
Error: Test failures
Execution halted

1 error x | 0 warnings √ | 0 notes √
Error in buildLibrary() :
The package check showed errors. You need to fix these errors first before submission!

`

Shouldn't the gdx test be shipped together with the library if you want to have an automated test?

Also, in order to not wait forever for the test to be done at every commit, couldn't you simply test the mif creation with just one region instead of all regions contained in the gdx file? This would require some adjustment to the convGDXMIF function but I don't really see the point of waiting forever to build the library for each commit, even if it is just a small change.

I would disable for now this feature and add it as a question in the buildLibrary script to make it easier to enable or disable and avoid these issues.

[giannou]

Shouldn't the gdx test be shipped together with the library if you want to have an automated test?

This is not possible unfortunately, that's why Alois implemented the automatic download.

Also, in order to not wait forever for the test to be done at every commit, couldn't you simply test the mif creation with just one region instead of all regions contained in the gdx file? This would require some adjustment to the convGDXMIF function but I don't really see the point of waiting forever to build the library for each commit, even if it is just a small change.

I think that tests should run for all regions (what if e.g. values only for a certain region don't add up?), or at lease people should be able to run the tests for all regions if they want to. The gain comes from the library remaining functional almost always, compared to now where every now and then it breaks and people have to restart jobs on the cluster etc etc. You lose some time waiting for the tests (which you can disable for fast intermediate build checks and leave for the last step before committing) but gain overall stability and thus, time.

I would disable for now this feature and add it as a question in the buildLibrary script to make it easier to enable or disable and avoid these issues.

Disabling the feature means people will just not do it so we are back at no testing. And buildlibrary is in essence exactly that, a question and check if the package builds correctly.

[Loisel]

@Loisel I am getting this error while trying to build the library:

`

== Failed tests ================================================================
-- Failure (test-convGDX2mif.R:46:5): Test if REMIND reporting is produced as it should and check data integrity --
md5sum(file.path(testgdx_folder, "fulldata.gdx")) not equal to "8ce7127ec26cb8bb36cecee3f4fa97f1".
names for target but not for current
-- Error (test-convGDX2mif.R:103:3): Test if REMIND reporting is produced as it should and check data integrity --
Error: Could not read GDX file ../testgdxs/fulldata.gdx: Expected data marker (SYMBOL_1) not found in GDX file (rc=0)
Backtrace:
x

1. -remind2:::runParallelTests(my_gdxs) test-convGDX2mif.R:103:2
2. -remind2::convGDX2MIF(i) test-convGDX2mif.R:95:8

3. \-remind2::toolRegionSubsets(gdx)
   

4.   \-gdx::readGDX(gdx, name = "regi2iso")
   

5.     \-gdxrrw::gdxInfo(path.expand(gdx), dump = FALSE, returnDF = TRUE)
   

[ FAIL 2 | WARN 0 | SKIP 0 | PASS 0 ]
Error: Test failures
Execution halted
1 error x | 0 warnings √ | 0 notes √
Error in buildLibrary() :
The package check showed errors. You need to fix these errors first before submission!

`

Shouldn't the gdx test be shipped together with the library if you want to have an automated test?

Also, in order to not wait forever for the test to be done at every commit, couldn't you simply test the mif creation with just one region instead of all regions contained in the gdx file? This would require some adjustment to the convGDXMIF function but I don't really see the point of waiting forever to build the library for each commit, even if it is just a small change.

I would disable for now this feature and add it as a question in the buildLibrary script to make it easier to enable or disable and avoid these issues.

An interesting catch. I suppose the problem is due to line endings on windows. I added a PR #5 which explicitly specifies "binary mode" for the downloads. Please try it out!

[Renato-Rodrigues]

I think that tests should run for all regions (what if e.g. values only for a certain region don't add up?), or at lease people should be able to run the tests for all regions if they want to. The gain comes from the library remaining functional almost always, compared to now where every now and then it breaks and people have to restart jobs on the cluster etc etc. You lose some time waiting for the tests (which you can disable for fast intermediate build checks and leave for the last step before committing) but gain overall stability and thus, time.

The convGDX2mif is region agnostic. That is a requirement to make it compatible with REMIND21 (REMIND-EU) and REMIND H12 at the same time. Testing it with a gdx with only two regions, 12 regions and/or 21 regions shouldn't make any difference except for the runtime to run the function. If that is not the case somehow, the code needs to be fixed.
As a positive externality, testing the function with less regions makes sure that no region dependable change is introduced to the library, which is a plus.
Therefore I keep my recommendation to use a smaller region subset to test the function, either through a test gdx with less regions, and/or an additional option to the convGDX2mif function parameters.

Disabling the feature means people will just not do it so we are back at no testing. And buildlibrary is in essence exactly that, a question and check if the package builds correctly.

I would still request, if it is not existent already, to add a parameter to the buildLibrary function to disable further tests if the user is aiming only debugging the library and not necessarily commit the changes yet.
I agree that the tests should be enabled by default. However, the way to disable the tests now at debugging time, having to comment an specific line in an specific file, is not a good code practice. You could even do not allow increasing the library version at the end of the buildLibrary evaluation, and issue a warning if the tests are disabled, but I am strongly in favor of having the option to control this when you are doing code debugging.

[Loisel]

I agree that the tests should be enabled by default. However, the way to disable the tests now at debugging time, having to comment an specific line in an specific file, is not a good code practice. You could even do not allow increasing the library version at the end of the buildLibrary evaluation, and issue a warning if the tests are disabled, but I am strongly in favor of having the option to control this when you are doing code debugging.

Why would you want to build the library then? Does "for debugging" mean, that you want to install & use the library? Please remember that you can always just devtools::load_all the library for testing purpose, no need to build!

[giannou]

Regions: I agree, the regions can be reduced to a small set of regions.

Disabling tests: For disabling the tests I am still not convinced that this is the right way to go, and opened an issue for further discussion: pik-piam/lucode2#7. I agree with Alois' comment above, in RStudio this is install & restart.

[Renato-Rodrigues]

@Loisel and @giannou
No, it is not the same as install & restart.
Install & restart do not test all things and does all dependency checks that buildLibrary tests.
One practical example, try to test the new DIETER reporting function. It is full of warnings when using buildLibrary, but it works in a simply install & restart scenario.
Removing these warnings and fixing the code for the DIETER function would take much longer if after each fix we would need to wait for the full gdx tests, compared to if you enable an easy way to simply use buildLibrary with tests disabled, and only test it against the gdx when you are close to commiting the change to the library.
I am really not asking something that complex, and I don't understand why adding a simple parameter wouldn't be better than the previous recommendation to comment lines in the code to do the same.

[giannou]

[ Since disabling the tests is not possible with buildlibrary: https://github.com/pik-piam/lucode2/issues/7 ] As I said before, in the current implementation of the REMIND reporting, automated tests is the only way to prevent breaking the remind2 package every now and then and loosing time restarting jobs and regenerating results. Some options I see:

• disable them and have no testing at all (the old default)
• keep them with the option of uncommenting one line in the code so that they only run before committing changes to github
• disable them for the user but leave them on in Github Actions (not sure if this will work as GAMS needs to be installed in the container running the tests), this way a PR will be rejected if it does not pass the tests
• initiate a process to move away from the current monolithic approach of testing the whole GDX2MIF function (will probably take more time but is good code practice).

[Loisel]

I'd opt for uncommenting the line. Advantages:

• it is some effort to the user to disable tests (I consider that a pro :)
• building (without uncommenting) locally does already produce the same result as the CI -> no need to re-iterate and patch a PR etc

@Renato-Rodrigues I see the point for a case where there are a lot of code check issues. Then, the code checks are required and should run quickly. But for (most) smaller changes I'd not expect many code check issues. For the exceptional case that there is an integration of larger chunks of code I'd say it is not asked too much to uncomment the line.

[Renato-Rodrigues]

My opinion is that the code line commenting is a hack, instead of a code feature.
In two months time somebody with a fish memory (thinking about myself, kkk) will not remember which file is the line that should be commented, or which is exactly the line to comment. Even if you do remember, anybody new to the library will never know about this until somebody expressly says that this is an "option".
If there is an option somewhere, either in a general library option or a function option, you will always have a help information that will describe what the parameter is for, and therefore the information on how to do tests with these checks disabled will be available for any newcomers or people with fish memory like myself.

[giannou]

In your Rstudio project options you can add an option to not run tests when you use the Check button (or Ctrl+Shift+E) in RStudio but the tests will run when using buidlibrary:

Would that work for you?

So you can basically do your code development and intermediate checking for warnings with Ctrl+Shift+E (faster than typing commands) and when you are ready to commit run buildLibrary

[Renato-Rodrigues]

I could give it a try, but my intention was never to disable every check, only the ones that rely in long gdx tests for now.

[tscheypidi]

as another idea: testthat comes with various functions to skip certain tests under certain condition (e.g. testthat::skip_if() or testthat::skip_on_os()). Perhaps you can create a fitting condition here under which the test should be skipped.

And if you want to make the tests faster but do not want to get of the full reporting test for now I want to second Renato here in his suggestion to test it only for a single region.

[giannou]

I could give it a try, but my intention was never to disable every check, only the ones that rely in long gdx tests for now.

The warnings that you were referring to before are still generated when you disable the tests. They come from problems in the R code, not from the calculations.

[giannou]

as another idea: testthat comes with various functions to skip certain tests under certain condition (e.g. testthat::skip_if() or testthat::skip_on_os()). Perhaps you can create a fitting condition here under which the test should be skipped.

And if you want to make the tests faster but do not want to get of the full reporting test for now I want to second Renato here in his suggestion to test it only for a single region.

I am aware of the skip_ functions but this would again mean a user has to actively change something somewhere to disable the slow tests, no?

As for the 1-region GDX, that'd be nice (but not sure how much it would speed up the slow tests), I am looking into ways to do that but it's not trivial.

[Loisel]

My opinion is that the code line commenting is a hack, instead of a code feature.
In two months time somebody with a fish memory (thinking about myself, kkk) will not remember which file is the line that should be commented, or which is exactly the line to comment. Even if you do remember, anybody new to the library will never know about this until somebody expressly says that this is an "option".
If there is an option somewhere, either in a general library option or a function option, you will always have a help information that will describe what the parameter is for, and therefore the information on how to do tests with these checks disabled will be available for any newcomers or people with fish memory like myself.

When I have outdated tests lying around for other projects, I often have to skip them for the time being. This always involves these kind of hacks. I also mentioned the possibility to uncomment this line in the README, but it is likely that nobody reads them. On the other hand, if we want to have some more test coverage in the future, people will get used to these kinds of "workaround", whether we like it or not.