Skip to content

Conversation

@smalltalkman
Copy link
Contributor

This PR enables v test vlib/v/builder/builder_test.v to pass the test on Windows systems below Win10.

The following are the test results before modification:

v test                      vlib/v/builder/builder_test.v
---- Testing... ----------------------------------------------------------------
 FAIL  2431.316 ms vlib/v/builder/builder_test.v
         retry: 1
      comp_cmd: "D:\gym\test\mingw-w64-packages\mingw-w64-v\src\v-weekly.2024.05\mingw-w64-x86_64-v.exe" -skip-running   -o "D:\gym\test\mingw-w64-packages\mingw-w64-v\tmp\mingw-w64-x86_64-v-weekly.2024.05-1\v_0\tsession_379c_25796335\01HNF86CWK22A0VZCBA79SRX6H\builder_test.exe" "D:\gym\test\mingw-w64-packages\mingw-w64-v\src\v-weekly.2024.05\vlib\v\builder\builder_test.v"
       run_cmd: "D:\gym\test\mingw-w64-packages\mingw-w64-v\tmp\mingw-w64-x86_64-v-weekly.2024.05-1\v_0\tsession_379c_25796335\01HNF86CWK22A0VZCBA79SRX6H\builder_test.exe"
failure code: 1; foutput.len: 732; failure output:
---------------------------------------------------------------------------------------------------- retry: 0 ; max_retry: 0 ; r.exit_code: 1 ; trimmed_output.len: 564
[D:\\gym\\test\\mingw-w64-packages\\mingw-w64-v\\src\\v-weekly.2024.05\\vlib\\v\\builder\\builder_test.v:27] original_file_list_: ['src']
[D:\\gym\\test\\mingw-w64-packages\\mingw-w64-v\\src\\v-weekly.2024.05\\vlib\\v\\builder\\builder_test.v:32] after_run_file_list: ['run_check.exe', 'src']
D:\\gym\\test\\mingw-w64-packages\\mingw-w64-v\\src\\v-weekly.2024.05\\vlib\\v\\builder\\builder_test.v:33: fn test_conditional_executable_removal
   > assert executable !in after_run_file_list
     Left value: run_check.exe
    Right value: ['run_check.exe', 'src']
--------------------------------------------------------------------------------

If time.sleep(200 * time.millisecond) is added to the test file, the test will also have a chance to succeed.

So the root of the problem is: executing os.execute('${os.quoted_path(vexe)} run .') and then executing os.ls(test_path)! will cause the file list to contain non-existent files. These non-existent files will be deleted delayed by Windows.

Here are some links related to the question:

@smalltalkman smalltalkman marked this pull request as draft February 6, 2024 12:33
@spytheman
Copy link
Member

Thank you for researching this. I think that it would be better to add a small delay before the os.ls() calls in the builder_test.v file instead, like you also noted.

The reason for that is that the proposed change here, adds an os.exists() call per each entry. I think that is a high cost, and it will still not eliminate the problem (the asynchronous deletion of files on older Windows versions, that breaks the expectations of the test code, according to golang/go#25965 (comment) ).

@smalltalkman
Copy link
Contributor Author

I think that it would be better to add a small delay before the os.ls() calls in the builder_test.v file instead, like you also noted.

My original idea was to add a delay to solve this problem, but the test results showed that there is still a chance of failure.

The reason for that is that the proposed change here, adds an os.exists() call per each entry. I think that is a high cost, and it will still not eliminate the problem (the asynchronous deletion of files on older Windows versions, that breaks the expectations of the test code, according to golang/go#25965 (comment) ).

Agree this is a time consuming scenario, maybe I should put the exists call into the test file.

I think after_run_file_list := os.ls(test_path)!.filter(os.exists(it)) is also reasonable in such a test scenario.

@smalltalkman smalltalkman changed the title os(windows): ignore non-existent files when listing files. test: make 'vlib/v/builder/builder_test.v' pass on older Windows versions. Feb 7, 2024
@smalltalkman smalltalkman marked this pull request as ready for review February 7, 2024 07:03
Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Excellent work.

@spytheman spytheman merged commit 349741d into vlang:master Feb 7, 2024
@smalltalkman smalltalkman deleted the fix-os_windows-ls-Ignore-non-existent-files branch February 7, 2024 09:04
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.

2 participants