Many Tests Failing

[HannahSi]

Many tests from design and core are failing during the Travis build (build #321)

In the graphing branch, which has everything in master and more, I fixed the errors caused by a misnaming of the K_KOZENY variable and incorrect import statements, but now many more errors have surfaced involving actual assert statements. Here's the summary of the tests:

@oliver-leung @ethan92429 I'd like to merge the graphing branch with the master branch soon and publish a release. Would fixing these errors in a few days be possible or would I be better off releasing the graphing code (which only requires procoda_parser.py) in a not-fully functioning package?

[eak24]

Hey Hannah! Definitely fix the errors. It's probably just that something is misnamed and thus throwing attribution errors or something like that. We're stepping backwards if we release a half working library.

On Mon, Oct 15, 2018, 01:23 Hannah Si notifications@github.com wrote:

Many tests from design and core are failing during the Travis build (build #321 https://travis-ci.org/AguaClara/aguaclara/builds/441473844)

In the graphing branch, which has everything in master and more, I fixed the errors caused by a misnaming of the K_KOZENY variable and incorrect import statements, but now many more errors have surfaced involving actual assert statements. Here's the summary of the tests:

https://user-images.githubusercontent.com/35944490/46929831-d9163c00-d00f-11e8-9507-fcd90e383e43.png

@oliver-leung https://github.com/oliver-leung @ethan92429 https://github.com/ethan92429 I'd like to merge the graphing branch with the master branch soon and publish a release. Would fixing these errors in a few days be possible or would I be better off releasing the graphing code (which only requires procoda_parser.py) in a not-fully functioning package?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AguaClara/aguaclara/issues/99, or mute the thread https://github.com/notifications/unsubscribe-auth/AIMAow142FOJdDqKWNjw1nNLWQAk_wXpks5ulBu3gaJpZM4Xbm2f .

[eak24]

I also see we're missing q all over the place... Unclear on how that happened. We don't want to sidestep tests.

On Mon, Oct 15, 2018, 09:04 Ethan Keller ethan.keller@gmail.com wrote:

Hey Hannah! Definitely fix the errors. It's probably just that something is misnamed and thus throwing attribution errors or something like that. We're stepping backwards if we release a half working library.

On Mon, Oct 15, 2018, 01:23 Hannah Si notifications@github.com wrote:

Many tests from design and core are failing during the Travis build (build #321 https://travis-ci.org/AguaClara/aguaclara/builds/441473844)

In the graphing branch, which has everything in master and more, I fixed the errors caused by a misnaming of the K_KOZENY variable and incorrect import statements, but now many more errors have surfaced involving actual assert statements. Here's the summary of the tests:

https://user-images.githubusercontent.com/35944490/46929831-d9163c00-d00f-11e8-9507-fcd90e383e43.png

@oliver-leung https://github.com/oliver-leung @ethan92429 https://github.com/ethan92429 I'd like to merge the graphing branch with the master branch soon and publish a release. Would fixing these errors in a few days be possible or would I be better off releasing the graphing code (which only requires procoda_parser.py) in a not-fully functioning package?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AguaClara/aguaclara/issues/99, or mute the thread https://github.com/notifications/unsubscribe-auth/AIMAow142FOJdDqKWNjw1nNLWQAk_wXpks5ulBu3gaJpZM4Xbm2f .

[HannahSi]

Thanks Ethan! I'd also like this release to fully pass its tests.

@oliver-leung Could the AIDE team see to resolving these errors?

[oliver-leung]

@HannahSi We'll take a look into it! Not sure how the tests failed, we simply renamed flow to q in some instances to conform to the naming conventions. It strangely says that the function call is missing q as a positional argument, even though it's called in the tests.

[fletchapin]

@HannahSi @oliver-leung I’ll take a look when I get time later this week as well

[fletchapin]

So after a few hours I finally figured out that the tests are failing because of the @ut.list_handler wrapper. I've created a new branch where I removed some of @ut.list_handler called fletch/fix_tests. However, I'm not sure why @ut.list_handler causes tests to fail

@ethan92429 does the @ut.list_handler require the function to have an additional argument for some reason? These are the results I got from testing with k_value_expansion() from head_loss.py

With @ut.list_handler, i.e. in it's current state: k_value_expansion(1, 2, 3) TypeError: k_value_expansion() missing 1 required positional argument: 'q'

k_value_expansion(1, 2, 3, 4) 0.3115797590560858 dimensionless

Once @ut.list_handler is removed: k_value_expansion(1, 2, 3) 0.5682953783234199 dimensionless

A few remaining tests have attribute errors because constants were removed during refactoring are called in the function and obviously not found. @oliver-leung the design team should look at those attribute errors and add the constants back into the package.

[oliver-leung]

I've fixed the attribute errors that @fletchapin started on fletch/fix_tests, but head_loss.py still claims that it's missing q as a positional argument. I renamed it from flow to q to conform to naming conventions - could that possibly have caused it?

[fletchapin]

@oliver-leung is there a @ut.list_handler wrapper? As I mentioned above the wrapper was leading to a bunch of positional argument exceptions (I'm not sure exactly why though, perhaps @ethan92429 would have a better idea)

[oliver-leung]

All of the tests use @ut.list_handler, but strangely, the only thing in common between the two tests that do pass is that they're both calls to k_value_orifice(pipe_id, orifice_id, orifice_l, q) with orifice_l == 0.

[fletchapin]

Have you tried removing @ut.list_handler from all the functions? That's what I did to get the tests in physchem to pass.

I still want to figure out why exactly that was causing an error before deciding if it's an easy fix or if @ut.list_handler should be removed

[fletchapin]

I've discovered the root cause of the issue with @ut.list_handler.

By printing out the inputs into the wrapper function I was able to see that the HandlerResult argument takes up the first argument into the function wrapped by list_handler. Therefore when list_handler calls the function after performing its pre-processing the *args variable is missing the first argument and the function throws a positional argument error.

[fletchapin]

Just updated list_handler and I believe it will pass all the tests now so long as the call is changed from @ut.list_handler to @ut.list_handler().

The reason being that I moved HandlerResult to be an optional argument of list_handler and moved func into the decorator. In this way if no argument is passed in HandlerResult="nparray" as before, but it doesn't take up one of the functions arguments as it's value since the call to wrapper is now wrapper(*args, **kwargs).

That might be a bit confusing so check out this version of list_handler and please test this fix out as I had very little chance to do so.

[HannahSi]

Discussion continued and (mostly) resolved in issue #103!