Fix Tests for new Decay on Absorb behavior

[dean-krueger]

This PR fixes the (/at least) two tests that seemed to be failing in the Cyclus build. The two failing tests were:

MaterialTest.AbsorbPrevDecay and ResourceTest.MaterialUnitValue

MaterialTest.AbsorbPrevDecay was fixed by adding a fake context to the simulation and then stepping through fakectx->time and checking that absorb records the correct timestamp (should be 11)

ResourceTest.MaterialUnitValue was failing because when you made the change in the material.cc Absorb function you accidentally deleted the part that averages the unit value of combined materials. I added that bit back and this test started passing! I guess this just goes to show why tests are so valuable! 😁

Either way, this PR should fix at least those two tests, and they were the ones I saw that were failing in your Cyclus PR, so hopefully this fixes that and then we're good to go!

[dean-krueger]

@gonuke Doesn't look like I have permission to add you as a reviewer, so pinging you like this so it (hopefully) shows up in your inbox

[gonuke]

Thanks for the fixes here @dean-krueger

All the other decay tests work without a context.

I think we may want to have some tests that just decay these different materials directly (ie. without a context) and then some that rely on the context.

[dean-krueger]

I'm not totally clear on what you mean by your comment, but I'll do my best to respond to what I think you mean?

The context is important for this specific test where it may not be for others because the new decay on absorb functionality is fundamentally linked to the time at which the materials are said to have been decayed. Since time lives in the context, we need to have it for this particular test. Since this test is specific to the behavior of materials decay when they are combined with the Absorb function, it seems appropriate to use the context here where is may not be for other tests (though it may also be, I'm not sure I didn't look at other tests, just this failing one).

[gonuke]

A couple of related things:

• the proposed change allows for abosrbing materials without a context, where the common decay time is simply the max of each of their previous decay times - we should probably test that behavior with materials that are not in a context, in addition to tests that rely on the context time (this was my original point)

• (something I realized while reviewing my own PR) the proposed change doesn't compare the current context time with the previous decay times, with the potential for the context time to be less than the previous decay times. This seems:

  1. unlikely, and
  2. almost certainly a bug if it occurs, but
  3. probably OK because decay can be done in reverse?

[dean-krueger]

The proposed change allows for abosrbing materials without a context, where the common decay time is simply the max of each of their previous decay times - we should probably test that behavior with materials that are not in a context, in addition to tests that rely on the context time (this was my original point)

A few questions about this:

1. When you say "the proposed change" are you talking about my fixes to the tests (the subject of this PR) or the change you made to the way the Material::Absorb function works (force both materials to decay on absorption cyclus/cyclus#1918). This question may seem pedantic, but I'm getting a little confused what we're talking about since this is a PR to fix tests into your PR to fix the Absorb function.
2. Do materials exist in Cyclus simulations without a context? The "Snapshot" materials we use, perhaps (I forget)?. If so, should those materials ever get decayed, since they're "snapshots" and as an extension does that test matter?
3. If yes to 2, circling back around to 1. is that something that matters for this PR into your PR? (I can see an argument for "we might as well do that since we're doing this", but I might also argue that keeping the scope of these things separate also has value so we don't wind up down a massive rabbit hole of recursively scope-creeped PRs into other PRs)

(something I realized while reviewing my own PR) the proposed change doesn't compare the current context time with the previous decay times, with the potential for the context time to be less than the previous decay times.

Is it possible (or more importantly "indented under normal operation") for Cyclus' context to go backwards in time? I didn't think it was, in which case I agree that this would be a bug, and would then argue that a material absorb test is probably not the place to make sure this isn't happening.

As always happy to do whatever, just wanted to make sure we were on the same page!

[gonuke]

Regarding the context:

• In the changes that I proposed in my PR force both materials to decay on absorption cyclus/cyclus#1918, there is the possibility for absorb to happen for materials that are not governed by a context.

• the previous behavior of Material::Absorb() didn't care about a context because it only picked the decay time of one of the two materials.

• I added a check for the context so that we could advance the decay time to the current context time, rather than only the times in the materials.

• I can't remember all the ways that we might arrive at materials being outside of a context, but most of the tests for decay throughout the material tests do not assume a context.

• if we only want to perform Absorb() on materials in a context, then Material::Absorb() needs to be updated to check that it's true and fail/warn otherwise.

• if we want to allow Absorb() for materials not in a context, then we should include tests for that.

[gonuke]

Regarding backward decay:

• I don't think it's every been considered a normal behavior

• if we always want to force decay to move forward we need to not just use the context time, but take the max of the previous decay times and the context time to determine the new time

• we can also fail/warn if the previous decay times are ever beyond the current context time

• if we leave it as is, it's possible that we cause decay to go backwards

• whatever we choose, we need to test it appropriately

[dean-krueger]

Gotcha. I'm going to advocate for moving this conversation/issue to the main PR and separating it from this PR, which simply aims to fix those two failing tests. I will make a comment in the main PR about adding additional tests and reference your explanations.

[dean-krueger]

Refactor these such that we use the REAL context (not the fake context). @gonuke and I talked about one or two other things maybe but I forget what they were. Perhaps I will remember while I'm fixing the first thing, or maybe he'll leave a comment if he remembers.