This solves the race condition but does not respect the original goal of not re-creating the data in every test method run. I think the intention of setup_class was that data creation would only happen once per parameterization.

There isn't really any way to parametrize setup_class except by setting a global variable (see this in the pytest documentation for what the setup_class interface is supposed to be), which can lead to issues when running tests in parallel.

I don't think it really effects the run-time performance of the unit tests meaningfully if that helps.

I wonder if the global variable definition can be moved to setup_module instead or is that also not parallel-friendly? I am not sure if a special masked fixture is even necessary in this case. Something like this:

MY_DATA = {}

def setup_module(module):
    global MY_DATA
    MYDATA = {'masked': Time(...), 'not_masked': Time(...)}

Then, the parameterized test method accesses MYDATA(use_mask)?

I wonder if the global variable definition can be moved to setup_module instead or is that also not parallel-friendly? I am not sure if a special masked fixture is even necessary in this case. Something like this:

MY_DATA = {}

def setup_module(module):
    global MY_DATA
    MYDATA = {'masked': Time(...), 'not_masked': Time(...)}

Then, the parameterized test method accesses MYDATA(use_mask)?

At this point one could just define setup_class where each variable is setup as a dictionary with keys 'masked' and 'not_masked', e.g. something like.

    def setup_class(cls):
        mjd = np.arange(50000, 50010)
        frac = np.arange(0., 0.999, 0.2)
        frac_masked =  np.ma.array(frac)
        frac_masked[1] = np.ma.masked
        cls.t0 = {
              'not_masked': Time(mjd[:, np.newaxis] + frac, format='mjd', scale='utc'),
              'masked': Time(mjd[:, np.newaxis] + frac_masked, format='mjd', scale='utc')
        }
     ...

@WilliamJamieson - I think this could be done with a far smaller change footprint. Instead of sticking with the original setup_class method, you can instead turn that into an instance method like setup_times(self, masked). This would then be very nearly the same as the original setup_class.

Then the slight annoyance is having to call self.setup_times(masked) at the start of each test method, but this is not too bad. The advantage is that overall the code stays cleaner and easier to read.

@WilliamJamieson - one point of process is that only the reviewer should be checking the review check boxes in the PR description. Making a final determination on each of the review points should wait until the code is ready (or nearly ready) to be merged.

Do the proposed changes actually accomplish desired goals?

@WilliamJamieson - can you address this in terms of a functional test that demonstrates the fix actually prevents parallel test failures consistently? Just passing CI once may not be sufficient. It looks like @pllim could consistently get a failure in a comment in #11966 so that would be a good thing to document.

@WilliamJamieson - I think this could be done with a far smaller change footprint. Instead of sticking with the original setup_class method, you can instead turn that into an instance method like setup_times(self, masked). This would then be very nearly the same as the original setup_class.

Then the slight annoyance is having to call self.setup_times(masked) at the start of each test method, but this is not too bad. The advantage is that overall the code stays cleaner and easier to read.

This is essentially what I did to start with in commit acb9858, @plim's initial comments lead me to believe that the regeneration of test data was undesirable. The further comment's lead me to make the changes you see now.

Please let me know if you would like me to roll back the to that commit.

Do the proposed changes actually accomplish desired goals?

@WilliamJamieson - can you address this in terms of a functional test that demonstrates the fix actually prevents parallel test failures consistently? Just passing CI once may not be sufficient. It looks like @pllim could consistently get a failure in a comment in #11966 so that would be a good thing to document.

Yes. When I initially discussed the race condition with @plim (off-line), with regard to the CI failures in PRs #11930 and #11931, I determined that I could replicate the failure on my local machine as described by the comment you allude to in #11966. I can demonstrate that merging the changes in this PR into the changes made by #11930 or #11931 always fixes the test failures I experience on my local machine.

Note that further exploration of the race condition shows that arbitrarily manipulating the tests added in #11930 or #11931 causes different tests in ~astropy.time.tests.test_methods.py to fail (including different numbers of tests), and that this PR fixes all of these scenarios as well. The failure is likely due to the fact that the global variable use_masked_data is getting overwritten by different parallel testing instances. I think my changes in #11930 and/or #11931 managed to mess with how tests where getting assigned to threads enough that tests in test_methods.py start getting assigned to different threads.

@WilliamJamieson - thanks for the explanation. I would actually be OK with your first commit, but I think we can accommodate both testing efficiency and keeping the code somewhat clean with a hybrid approach of the first two commits:

• Define the test data once with a setup_class, just like the current code in commit 2
• Call the create_data method in each test to define the data from the class attributes, e.g. self.t0 = self.__class__.t0[use_mask] etc.

I think this will be quick to implement if you go start from your first commit and then copy the setup_class from your second commit into the test file and finally update create_data accordingly. You should include some code comments that discuss the driver for this structure (supporting parallel testing without needlessly recreating test data).

@WilliamJamieson - thanks for the explanation. I would actually be OK with your first commit, but I think we can accommodate both testing efficiency and keeping the code somewhat clean with a hybrid approach of the first two commits:

• Define the test data once with a setup_class, just like the current code in commit 2
• Call the create_data method in each test to define the data from the class attributes, e.g. self.t0 = self.__class__.t0[use_mask] etc.

I think this will be quick to implement if you go start from your first commit and then copy the setup_class from your second commit into the test file and finally update create_data accordingly. You should include some code comments that discuss the driver for this structure (supporting parallel testing without needlessly recreating test data).

I implemented this in my latest commit: 2212098. The changes still fix the error in #11930 and #11931.

@WilliamJamieson - awesome, this is great from looking at the code, but now I see that the first CI test has failed. Once you get tests passing I will merge this.

@pllim - this seems like a useful bug fix to backport to 4.3.x so I set the milestone to 4.3.2. Is that OK? The original code that brought in this issue is old, from 2017.

Is that OK?

Sure! It is up to the discretion of the subpackage maintainer, usually. 😉

The CI failure makes no sense to me... Maybe I'll restart to make sure it is not transient.

Sure! It is up to the discretion of the subpackage maintainer, usually. 😉

@pllim - OK good. I was guessing that you originally set the milestone to 5.0 so I thought there might be a reason.

originally set the milestone to 5.0

Because this blocks a PR that is milestoned to 5.0. So, that is the minimum required milestone.