Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test/net crash test #786

Merged
merged 3 commits into from
Apr 17, 2023
Merged

Conversation

Jeevananthan-23
Copy link
Contributor

Closes #768

Copy link
Contributor

@laimis laimis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to run this locally end to end? For me, once I modified the working directory path to match what's on my machine, I was able to start the test, but when the test gets invoked via Process.Start, dotnet test is encountering locking issues:

C:\Program Files\dotnet\sdk\7.0.104\Microsoft.Common.CurrentVersion.targets(4862,5): error MSB3027: Could not copy "....\Lucene.Net.TestFramework.dll" to "bin\Debug\net7.0\Lucene.Net.TestFramework.dll". Exceeded retry count of 10. Failed. The file is locked by: "testhost (5360)" [..\src\Lucene.Net.Tests._I-J\Lucene.Net.Tests._I-J.csproj]

…ation to crash the test upon timeout and log the results. Added missing support for the tmpDir system property when creating temp folders and files so we can pass the from one process to another.
@NightOwl888
Copy link
Contributor

Thanks for the PR.

I went through and completed this task, as it was clearly going to take a lot of back and forth to work out, especially being that I admittedly didn't have a complete picture of what the test was doing. Several things it does are very subtle and you have to consider the implementation of how the test framework functions in order to work them out.

  1. The test has no asserts to verify the index. The test passes whether CheckIndexes() returns true or false. This was confusing at first, but the TestUtil.CheckIndex() method will throw an exception if it detects index corruption, which is the failure condition we are looking for.
  2. Passing the directory path to the forked test was a bit of a challenge. Especially being that the tmpDir system property hadn't been implemented and the test internally calls GetTempDirectory() with no way to override. In fact, the test framework is still missing a few features. So, this test framework feature had to be implemented to get this test to function.
  3. The test framework automatically deletes the index directory at the end of a test run (and technically, it is cleaned up prior to that at the end of the TestNRTThreads_Mem() method. This had me scratching my head for awhile as to how we would have any results to test on disk. The test relies on the crash to ensure the index stays persisted. This also means there will only ever by a single index (or no index) to check even though the base class test is called in a loop several times.
  4. Being that the test runs inside NUnit and NUnit has safeguards in place to ensure an uncaught exception doesn't bring down the test runner, it would have been challenging to bypass this behavior. It turned out to be simpler to crash the process from the outside using Process.Kill(). Unfortunately, Process.Kill() doesn't function to kill the current process, only another process.
  5. Getting the process id of the process to kill was also challenging. dotnet test runs an executable named vstest.console.exe. This executable in turn runs testhost.exe which runs the tests. It is the latter we need to kill, not the process that we originally started. I went down several wrong turns before I realized that the process id could simply be logged to a file and then read back from our original test so it could start the thread to kill it after x amount of time. However, that does mean another shared path that we have to create in the original test and pass the file name to the fork.
  6. I originally suggested using environment variables to configure system properties locally. However, in our CI environments we create a lucene.testSettings.json file rather than passing them in on the command line. This configuration file overrides any environment variables and it applies to all tests in all subdirectories where it exists. So, for the settings to override the config file, we needed to pass the settings on the command line and the syntax for doing so is very ugly and requires escaping. Fortunately, because we are using UseShellExecute = false we don't have any inconsistency between Windows, Linux, and macOS, but it does require a different syntax to properly escape in C# in order to pass the correct escape characters into dotnet test.

Ultimately, we got it done, though. It took a lot of research and analysis to pull off.

Do note that creating the directory has been moved inside of the loop because I realized that if it were left a single shared directory all of the loops would put results into the same temp directory that is checked. And since they are checked in lexicographical order and always return on the first index checked, there is a high probability that if the first check fails most of the rest of the checks would also fail.

@NightOwl888 NightOwl888 merged commit f6b33b9 into apache:master Apr 17, 2023
198 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port missing test: TestIndexWriterOnJRECrash
3 participants