Make an event to test OG-USA + B-Tax on Tax-Calc master change

[PeterDSteinberg]

• Run get_micro_data.py from OG-USA
• Run B-Tax regression

Use Travis CI environment variables and webhook options to test these just on changes to master branch.

[MattHJensen]

I think it would be preferable to run these from og-USA and b-tax respectively on new taxcalc releases. In tax-calculator, it would be best to find the most parsimonious tests possible that would identify breaking changes for b-tax and og-USA so that we don't dramatically affect the time it takes to run the taxcalc test suite.

[jdebacker]

As an FYI, the breaking changes that I've noticed in the past have been changes to variable names, changes to the variables available in the PUF, and changes to the list of variables for which the mtr function is available.

[PeterDSteinberg]

There have also been breaking changes in the deploy repo - the parts where the interface (runner) functions are called and are inconsistent with changes in the Tax-Calculator, B-Tax, and OG-USA repos. I'll think further how we may better test that integration as well.

[PeterDSteinberg]

Here's the related B-Tax regression testing PR: PSLmodels/Cost-of-Capital-Calculator#150

[PeterDSteinberg]

Is there any interest, @jdebacker, @MattHJensen @martinholmer , in have private puf.csv.gz-related tests running in Travis? There is a way we can encrypt things to allow an install of taxpuf on Travis CI tests safely. If so, I can add that to the checklist at the top of this issue.

[jdebacker]

@PeterDSteinberg Let me be sure I understand.

I thought we currently did Travis CI testing of OG-USA and B-Tax with a puf.csv.gz file. Is the right?

I thought the current puf.csv.gz file was fake data for testing - and that it did not have to be private like the actually puf.csv file. Do I have this wrong?

[PeterDSteinberg]

Correct, @jdebacker , I forgot about that - we are using a faked puf.csv.gz in CI tests. I meant to say that we could run Travis CI tests with the private actual puf.csv.gz safely with respect to the actual puf.csv.gz's privacy. We wouldn't have to commit any taxpuf package related security details to the repo(s) to do so but rather encrypt them in Travis CI.

[jdebacker]

Ok- then how about run time? Would it be about the same with the actual puf file? If so, I guess we move to that - it would save some errors we've seen where we testing locally with actual puf, but had Travis CI failures because we didn't have a faked puf that was the same in the repo where the tests were run from.

[MattHJensen]

Is there any interest, @jdebacker, @MattHJensen @martinholmer , in have private puf.csv.gz-related tests running in Travis? There is a way we can encrypt things to allow an install of taxpuf on Travis CI tests safely. If so, I can add that to the checklist at the top of this issue.

I believe this would be a security risk (see a previous discussion, here).

The problem is that a non-core maintainer could write a test that discloses information about the puf. A PR containing the new test would trigger TravisCI before any core maintainer has reviewed the test.

[PeterDSteinberg]

Got it...That makes sense @MattHJensen

[jdebacker]

Looping in @rickecon here. We just talked and he had an idea about testing: Put together a list of variables that the dependent programs (OG-USA, B-Tax, etc) need from TC. Then run a test with each new version of TC to make sure those variables are output.

As noted above, this would catch most changes. I'll provide a list from B-Tax below.

But we do need to be aware that such a test would not account for changes in syntax in the calls to TC and the calculator class. These are not common, but I've run into issues when there were changes in how growth parameters were handled, for example. These type of changes cause exceptions that stop model runs.

We'd also still need to be sure that any individual reforms passed to TC from these models for regression testing properly account for changes in the format used in the reform dictionary. These are common changes, but I don't think we can set up any automatic testing to account for the presence of such changes. Errors of this type cause the output to not make sense, but don't always cause exceptions that prevent the model run (it depends what changes with the reform dictionary format).

cc @MattHJensen

[jdebacker]

B-Tax uses the following from the calculator class (where calc1 is a specific instance of the calculator class):

calc1.mtr('e00900p')
calc1.mtr('e02000')
calc1.mtr('e26270')
calc1.mtr('e00650')
calc1.mtr('e00300')
calc1.mtr('e00400')
calc1.mtr('p22250')
calc1.mtr('p23250')
calc1.mtr('e19200')
calc1.mtr('e18500')
calc1.records.e00900p
calc1.records.e02000
calc1.records.c04800
calc1.records.e00900p
calc1.records.e26270
calc1.records.s006
calc1.records.e00650
calc1.records.e00300 
calc1.records.p22250
calc1.records.p23250
calc1.records.e01500
calc1.records.e19200
calc1.records.e18500

[rickecon]

OG-USA uses the following from the calculator class (where calc1 is a specific instance of the calculator class):

calc1.mtr('e00200p')
calc1.mtr('e00300')
calc1.mtr('e00400')
calc1.mtr('e00600')
calc1.mtr('e00650')
calc1.mtr('e00900p')
calc1.mtr('e01400')
calc1.mtr('e01700')
calc1.mtr('e02000')
calc1.mtr('e22250')
calc1.mtr('e23250')
calc1.records.e00300
calc1.records.e00400
calc1.records.e00600
calc1.records.e00650
calc1.records.e01400
calc1.records.e01700
calc1.records.e02000
calc1.records.e22250
calc1.records.e23250

These are the variables called in the get_micro_data.py module. @jdebacker @MattHJensen

[MattHJensen]

@rickecon, could you take another look at the OG-USA list? It looks like it is missing several variables I would expect, like s006, _combined, etc.

[martinholmer]

Where do we stand on the conversation in Tax-Calculator issue #1275?

Nobody has commented on this issue in almost a month.

Are there specific things (not mentioned elsewhere) that need to be done concerning #1275?

If not, should #1275 be closed?

@MattHJensen @PeterDSteinberg @jdebacker @rickecon

[jdebacker]

@martinholmer I think we should keep this issue open. It needs to be done, but my understanding is that the progress has been slowed because web-app public Issue #543 integrally relates. This issue has ramifications for how data are to be standardized for passing between the various models and thus should be addressed before we build the testing platform for interactions across models.

[martinholmer]

@jdebacker said:

I think we should keep this issue open. It needs to be done, but my understanding is that the progress has been slowed because web-app public Issue #543 integrally relates. This issue has ramifications for how data are to be standardized for passing between the various models and thus should be addressed before we build the testing platform for interactions across models.

OK. Thanks for the progress report on issue #1275.

[martinholmer]

@MattHJensen said on April 3, 2017, about Tax-Calculator issue #1275:

I think it would be preferable to run these [tests] from OG-USA and B-Tax, respectively, on new taxcalc releases.

This approach is now explicit in the new Release Process described in webapp-public issue 560.

Also, Tax-Calculator release numbering has adopted semantic versioning to signal when the public API changes. And there is now a Tax-Calculator RELEASES.md document that lists API changes that would cause projects using Tax-Calculator to review their use of the Tax-Calculator API.

Given all these developments since early April, Tax-Calculator issue #1275 is being closed.