You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@daffodil.apache.org by Steve Lawrence <sl...@apache.org> on 2020/10/26 13:54:58 UTC
TestDaffodilXMLLoader non-thread safe tests
We have three tests in TestDaffodilXMLLoader that had a random failure
in Github Actions for a pull request I created that updates SBT to
1.4.1. Looking at these tests, I believe they are not thread-safe and
that's likely causing the failure I'm seeing.
The way they work is each tests creates a schema, test data, and XML
Catalog file, and a Catalog.properties file.
The first issue is each test writes to the exact same file location, so
if these individual tests are run in parallel things will break. One
option to resolve this would be to create files in random temp
directories. This would work except for the next issue.
Which is that the test creates a CatalogManager.properties file at the
root the classpath. When Daffodil does XML resolution, it searches for
this file to change specify XML catalogs. This file is temporarily added
during these tests to reference the created XML Catalog file
This means that while these tests are running, the
CatalogManager.properties file could affect any other tests, not just
those in this TestDaffodilXMLLoader file. I'm wondering if this was the
actual cause of some of the random failures we used to see fairly
regularly and thought it was just non-thread safe code in the TDML
Runner. Perhaps it was actually just this test changing
CatalogManager.properties and breaking XML resolution.
I can't think of a safe way to deal with testing this capability, except
maybe CLI tests where we actually fork processes and could do so in temp
directories with custom properties files. But these tests seem to be a
bit more unit-testy. Any thoughts on how to test this capability? Should
we convert to CLI test? Is there an alternative? Or should we just
remove them completely?
Re: TestDaffodilXMLLoader non-thread safe tests
Posted by Steve Lawrence <sl...@apache.org>.
Unfortunately, I'm not sure that's an option. We don't actually have any
code to handle CatalogManager.properties file. The logic in
DaffodilXMLLoader.scala just creates a 'new Catalog', which relies on
the built in CatalogManager, and I don't see any way to specify an
alternate file for the .properties file.
Though, the fact that the CatalogManager logic is all handled by another
library, perhaps these tests aren't as important. I guess we could be
using this library incorrectly, but hopefully there are at least no bugs
in that library.
On 10/26/20 11:33 AM, Interrante, John A (GE Research, US) wrote:
> I'm leaning towards +1 for removing these tests. If the reason for keeping them is good enough (to prevent regressions in the code being tested), then I would suggest an easier change than creating a separate test module or making the tests CLI tests. The tests could create files in random temp directories (which would fix the first issue) and the code being tested could have a conditional branch added to make the code read a CatalogManagerTest.properties at the root of the classpath (which would fix the second issue). I haven't seen the code being tested so I'm not sure how to make that branch conditional, but the idea is to somehow make the code and unit test communicate with each other (ah, I'm being tested, so I'll read that file over there instead of this file here).
>
> -----Original Message-----
> From: Steve Lawrence <sl...@apache.org>
> Sent: Monday, October 26, 2020 11:04 AM
> To: dev@daffodil.apache.org
> Subject: EXT: Re: TestDaffodilXMLLoader non-thread safe tests
>
> Yes, but it feels a bit wasteful to create a new module for three tests.
> Note that one of the tests is just testing that the test rig in this file works correctly. So there's really only two tests here.
>
> Also note that we can't change the working directory when forked per test, so each test would need to share the same Catalog.properties file, which I don't think would work.
>
> I think the best way to test things work is via a CLI test that can change directory, create files, have a custom Catalog.properties file, etc.
>
> Though, these tests don't even use valid DFDL schemas. It really just tests a couple specific things about the XMLLoader. So it might be a bit of work to get it working end ensuring it's testing the right thing, if that's even possible.
>
>
> On 10/26/20 10:40 AM, Beckerle, Mike wrote:
>> Is there any way to have a separate test module where tests are all run with fork = true?
>> ________________________________
>> From: Steve Lawrence <sl...@apache.org>
>> Sent: Monday, October 26, 2020 9:54 AM
>> To: dev@daffodil.apache.org <de...@daffodil.apache.org>
>> Subject: TestDaffodilXMLLoader non-thread safe tests
>>
>> We have three tests in TestDaffodilXMLLoader that had a random failure
>> in Github Actions for a pull request I created that updates SBT to
>> 1.4.1. Looking at these tests, I believe they are not thread-safe and
>> that's likely causing the failure I'm seeing.
>>
>> The way they work is each tests creates a schema, test data, and XML
>> Catalog file, and a Catalog.properties file.
>>
>> The first issue is each test writes to the exact same file location,
>> so if these individual tests are run in parallel things will break.
>> One option to resolve this would be to create files in random temp
>> directories. This would work except for the next issue.
>>
>> Which is that the test creates a CatalogManager.properties file at the
>> root the classpath. When Daffodil does XML resolution, it searches for
>> this file to change specify XML catalogs. This file is temporarily
>> added during these tests to reference the created XML Catalog file
>>
>> This means that while these tests are running, the
>> CatalogManager.properties file could affect any other tests, not just
>> those in this TestDaffodilXMLLoader file. I'm wondering if this was
>> the actual cause of some of the random failures we used to see fairly
>> regularly and thought it was just non-thread safe code in the TDML
>> Runner. Perhaps it was actually just this test changing
>> CatalogManager.properties and breaking XML resolution.
>>
>> I can't think of a safe way to deal with testing this capability,
>> except maybe CLI tests where we actually fork processes and could do
>> so in temp directories with custom properties files. But these tests
>> seem to be a bit more unit-testy. Any thoughts on how to test this
>> capability? Should we convert to CLI test? Is there an alternative? Or
>> should we just remove them completely?
>>
>
TestDaffodilXMLLoader non-thread safe tests
Posted by "Interrante, John A (GE Research, US)" <in...@research.ge.com>.
I'm leaning towards +1 for removing these tests. If the reason for keeping them is good enough (to prevent regressions in the code being tested), then I would suggest an easier change than creating a separate test module or making the tests CLI tests. The tests could create files in random temp directories (which would fix the first issue) and the code being tested could have a conditional branch added to make the code read a CatalogManagerTest.properties at the root of the classpath (which would fix the second issue). I haven't seen the code being tested so I'm not sure how to make that branch conditional, but the idea is to somehow make the code and unit test communicate with each other (ah, I'm being tested, so I'll read that file over there instead of this file here).
-----Original Message-----
From: Steve Lawrence <sl...@apache.org>
Sent: Monday, October 26, 2020 11:04 AM
To: dev@daffodil.apache.org
Subject: EXT: Re: TestDaffodilXMLLoader non-thread safe tests
Yes, but it feels a bit wasteful to create a new module for three tests.
Note that one of the tests is just testing that the test rig in this file works correctly. So there's really only two tests here.
Also note that we can't change the working directory when forked per test, so each test would need to share the same Catalog.properties file, which I don't think would work.
I think the best way to test things work is via a CLI test that can change directory, create files, have a custom Catalog.properties file, etc.
Though, these tests don't even use valid DFDL schemas. It really just tests a couple specific things about the XMLLoader. So it might be a bit of work to get it working end ensuring it's testing the right thing, if that's even possible.
On 10/26/20 10:40 AM, Beckerle, Mike wrote:
> Is there any way to have a separate test module where tests are all run with fork = true?
> ________________________________
> From: Steve Lawrence <sl...@apache.org>
> Sent: Monday, October 26, 2020 9:54 AM
> To: dev@daffodil.apache.org <de...@daffodil.apache.org>
> Subject: TestDaffodilXMLLoader non-thread safe tests
>
> We have three tests in TestDaffodilXMLLoader that had a random failure
> in Github Actions for a pull request I created that updates SBT to
> 1.4.1. Looking at these tests, I believe they are not thread-safe and
> that's likely causing the failure I'm seeing.
>
> The way they work is each tests creates a schema, test data, and XML
> Catalog file, and a Catalog.properties file.
>
> The first issue is each test writes to the exact same file location,
> so if these individual tests are run in parallel things will break.
> One option to resolve this would be to create files in random temp
> directories. This would work except for the next issue.
>
> Which is that the test creates a CatalogManager.properties file at the
> root the classpath. When Daffodil does XML resolution, it searches for
> this file to change specify XML catalogs. This file is temporarily
> added during these tests to reference the created XML Catalog file
>
> This means that while these tests are running, the
> CatalogManager.properties file could affect any other tests, not just
> those in this TestDaffodilXMLLoader file. I'm wondering if this was
> the actual cause of some of the random failures we used to see fairly
> regularly and thought it was just non-thread safe code in the TDML
> Runner. Perhaps it was actually just this test changing
> CatalogManager.properties and breaking XML resolution.
>
> I can't think of a safe way to deal with testing this capability,
> except maybe CLI tests where we actually fork processes and could do
> so in temp directories with custom properties files. But these tests
> seem to be a bit more unit-testy. Any thoughts on how to test this
> capability? Should we convert to CLI test? Is there an alternative? Or
> should we just remove them completely?
>
Re: TestDaffodilXMLLoader non-thread safe tests
Posted by Steve Lawrence <sl...@apache.org>.
Yes, but it feels a bit wasteful to create a new module for three tests.
Note that one of the tests is just testing that the test rig in this
file works correctly. So there's really only two tests here.
Also note that we can't change the working directory when forked per
test, so each test would need to share the same Catalog.properties file,
which I don't think would work.
I think the best way to test things work is via a CLI test that can
change directory, create files, have a custom Catalog.properties file, etc.
Though, these tests don't even use valid DFDL schemas. It really just
tests a couple specific things about the XMLLoader. So it might be a bit
of work to get it working end ensuring it's testing the right thing, if
that's even possible.
On 10/26/20 10:40 AM, Beckerle, Mike wrote:
> Is there any way to have a separate test module where tests are all run with fork = true?
> ________________________________
> From: Steve Lawrence <sl...@apache.org>
> Sent: Monday, October 26, 2020 9:54 AM
> To: dev@daffodil.apache.org <de...@daffodil.apache.org>
> Subject: TestDaffodilXMLLoader non-thread safe tests
>
> We have three tests in TestDaffodilXMLLoader that had a random failure
> in Github Actions for a pull request I created that updates SBT to
> 1.4.1. Looking at these tests, I believe they are not thread-safe and
> that's likely causing the failure I'm seeing.
>
> The way they work is each tests creates a schema, test data, and XML
> Catalog file, and a Catalog.properties file.
>
> The first issue is each test writes to the exact same file location, so
> if these individual tests are run in parallel things will break. One
> option to resolve this would be to create files in random temp
> directories. This would work except for the next issue.
>
> Which is that the test creates a CatalogManager.properties file at the
> root the classpath. When Daffodil does XML resolution, it searches for
> this file to change specify XML catalogs. This file is temporarily added
> during these tests to reference the created XML Catalog file
>
> This means that while these tests are running, the
> CatalogManager.properties file could affect any other tests, not just
> those in this TestDaffodilXMLLoader file. I'm wondering if this was the
> actual cause of some of the random failures we used to see fairly
> regularly and thought it was just non-thread safe code in the TDML
> Runner. Perhaps it was actually just this test changing
> CatalogManager.properties and breaking XML resolution.
>
> I can't think of a safe way to deal with testing this capability, except
> maybe CLI tests where we actually fork processes and could do so in temp
> directories with custom properties files. But these tests seem to be a
> bit more unit-testy. Any thoughts on how to test this capability? Should
> we convert to CLI test? Is there an alternative? Or should we just
> remove them completely?
>
Re: TestDaffodilXMLLoader non-thread safe tests
Posted by "Beckerle, Mike" <mb...@owlcyberdefense.com>.
Is there any way to have a separate test module where tests are all run with fork = true?
________________________________
From: Steve Lawrence <sl...@apache.org>
Sent: Monday, October 26, 2020 9:54 AM
To: dev@daffodil.apache.org <de...@daffodil.apache.org>
Subject: TestDaffodilXMLLoader non-thread safe tests
We have three tests in TestDaffodilXMLLoader that had a random failure
in Github Actions for a pull request I created that updates SBT to
1.4.1. Looking at these tests, I believe they are not thread-safe and
that's likely causing the failure I'm seeing.
The way they work is each tests creates a schema, test data, and XML
Catalog file, and a Catalog.properties file.
The first issue is each test writes to the exact same file location, so
if these individual tests are run in parallel things will break. One
option to resolve this would be to create files in random temp
directories. This would work except for the next issue.
Which is that the test creates a CatalogManager.properties file at the
root the classpath. When Daffodil does XML resolution, it searches for
this file to change specify XML catalogs. This file is temporarily added
during these tests to reference the created XML Catalog file
This means that while these tests are running, the
CatalogManager.properties file could affect any other tests, not just
those in this TestDaffodilXMLLoader file. I'm wondering if this was the
actual cause of some of the random failures we used to see fairly
regularly and thought it was just non-thread safe code in the TDML
Runner. Perhaps it was actually just this test changing
CatalogManager.properties and breaking XML resolution.
I can't think of a safe way to deal with testing this capability, except
maybe CLI tests where we actually fork processes and could do so in temp
directories with custom properties files. But these tests seem to be a
bit more unit-testy. Any thoughts on how to test this capability? Should
we convert to CLI test? Is there an alternative? Or should we just
remove them completely?