Skip to content

fix(cli): avoid race condition preventing reporting some queries #581

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

Merged
merged 2 commits into from
Jun 8, 2025

Conversation

hlidotbe
Copy link
Contributor

@hlidotbe hlidotbe commented Jun 7, 2025

Listing the queries in the basic demo correctly yields 6 queries:

kulala_cli.lua 1.basic.http --list   

File: /app/kulala.nvim/docs/static/import/demos/1.basic.http
Line Name                                    URL                                               
 4    Simple GET request                      GET https://httpbin.org/get                      
 8    GET with query parameters               GET https://httpbin.org/get?name=kulala&age=25   
 12   POST with JSON body                     POST https://httpbin.org/post                    
 22   PUT with headers                        PUT https://httpbin.org/put                      
 33   DELETE request                          DELETE https://httpbin.org/delete                
 37   GET with multiple headers               GET https://httpbin.org/headers                  

but running the tests only finished 5:

kulala_cli.lua 1.basic.http -v report

..................................................*...............................................*...............................................................*...............................
................*..............................................*

Line URL                                               Status  Time      Duration       

 4    https://httpbin.org/get                           200     11:46:29  404.15 ms     

 8    https://httpbin.org/get?name=kulala&age=25        200     11:46:30  518.17 ms     

 12   https://httpbin.org/post                          200     11:46:32  1642.42 ms    

 22   https://httpbin.org/put                           200     11:46:32  414.43 ms     

 33   https://httpbin.org/delete                        200     11:46:32  404.90 ms     

Summary             Total               Successful          Failed              
 Requests            5                   5                   0                  
 Asserts             0                   0                   0                  
.
Status: OK

This is because is_last returns true when all tasks are scheduled but do not check if they are actually done. If the last task is not finished quickly enough then it is not reported.

This fix checks if the amount of tasks done is equal to the total tasks scheduled

@hlidotbe
Copy link
Contributor Author

hlidotbe commented Jun 7, 2025

Well it doesn't seems good enough. When running a single file it's ok but when running a test folder it mixes up the test from one file into the other...

@YaroSpace
Copy link
Collaborator

YaroSpace commented Jun 7, 2025

Hm, I remember this issue. I thought I fixed it.
Will have a look.

@YaroSpace YaroSpace self-assigned this Jun 7, 2025
Listing the queries in the basic demo correctly yields 6 queries:

```
kulala_cli.lua 1.basic.http --list   

File: /app/kulala.nvim/docs/static/import/demos/1.basic.http
Line Name                                    URL                                               
 4    Simple GET request                      GET https://httpbin.org/get                      
 8    GET with query parameters               GET https://httpbin.org/get?name=kulala&age=25   
 12   POST with JSON body                     POST https://httpbin.org/post                    
 22   PUT with headers                        PUT https://httpbin.org/put                      
 33   DELETE request                          DELETE https://httpbin.org/delete                
 37   GET with multiple headers               GET https://httpbin.org/headers                  
```

but running the tests only finished 5:

```
kulala_cli.lua 1.basic.http -v report

..................................................*...............................................*...............................................................*...............................
................*..............................................*

Line URL                                               Status  Time      Duration       

 4    https://httpbin.org/get                           200     11:46:29  404.15 ms     

 8    https://httpbin.org/get?name=kulala&age=25        200     11:46:30  518.17 ms     

 12   https://httpbin.org/post                          200     11:46:32  1642.42 ms    

 22   https://httpbin.org/put                           200     11:46:32  414.43 ms     

 33   https://httpbin.org/delete                        200     11:46:32  404.90 ms     

Summary             Total               Successful          Failed              
 Requests            5                   5                   0                  
 Asserts             0                   0                   0                  
.
Status: OK
```

This is because is_last returns true when all tasks are scheduled but do not check if they are actually done. If the last task is not finished quickly enough then it is not reported.

This fix checks if the amount of tasks done is equal to the total tasks scheduled
@YaroSpace YaroSpace force-pushed the fix/cli-race-condition branch from 4664b22 to fbe95fe Compare June 7, 2025 22:20
@YaroSpace YaroSpace changed the base branch from main to develop June 7, 2025 22:20
@YaroSpace
Copy link
Collaborator

YaroSpace commented Jun 7, 2025

Well it doesn't seems good enough. When running a single file it's ok but when running a test folder it mixes up the test from one file into the other...

I have checked, the fix works fine for me.
The cumulative print out was by design, but I changed that.

Btw, I changed the base to develop.
Thank you very much for your contribution!

YaroSpace
YaroSpace previously approved these changes Jun 7, 2025
@YaroSpace YaroSpace force-pushed the fix/cli-race-condition branch from c76baa0 to 2f5979f Compare June 7, 2025 22:42
@hlidotbe
Copy link
Contributor Author

hlidotbe commented Jun 8, 2025

If I run tests on a folder with my fix the reports "accumulates":


.......................................................*

Line URL                                               Status  Time      Duration       

 4    https://www.epic.net/en/article/showreel-2024/    200     06:05:54  662.57 ms     

Summary             Total               Successful          Failed              
 Requests            1                   1                   0                  
 Asserts             0                   0                   0                  
.
Status: OK
........................................................*

Line URL                                               Status  Time      Duration       

 4    https://www.epic.net/en/article/showreel-2024/    200     06:05:54  662.57 ms     

 4    https://www.epic.net/en/contact/new-project/      200     06:05:54  592.60 ms     

  Should have the right prompt: expected "Tell us about your project", got "  <!DOCTYPE html><html  data-head-attrs=""  lan..."

Summary             Total               Successful          Failed              
 Requests            2                   2                   0                  
 Asserts             1                   1                   0                  
.
Status: OK
.............................................*...........................................*.........................................................*

Line URL                                               Status  Time      Duration       

 4    https://www.epic.net/en/article/showreel-2024/    200     06:05:54  662.57 ms     

 4    https://www.epic.net/en/contact/new-project/      200     06:05:54  592.60 ms     

  Should have the right prompt: expected "Tell us about your project", got "  <!DOCTYPE html><html  data-head-attrs=""  lan..."

 4    https://www.epic.net/                             301     06:05:55  89.51 ms      

  Should redirect permanently: expected "301", got "301"
  Should redirect to /en/: expected "/en/", got "/en/"

 14   https://www.epic.net/                             301     06:05:55  86.51 ms      

  Should redirect permanently: expected "301", got "301"
  Should redirect to /fr/: expected "/fr/", got "/fr/"

 24   https://www.epic.net/fr/                          200     06:05:56  628.82 ms     

  Should have 'Projets': expected "Projets", got "  <!DOCTYPE html><html  data-head-attrs=""  lan..."
  Should have 'Conditions Générales': expected "Conditions générales", got "  <!DOCTYPE html><html  data-head-attrs=""  lan..."

Summary             Total               Successful          Failed              
 Requests            5                   5                   0                  
 Asserts             7                   7                   0                  
.
Status: OK

There's two files with one test each and one with 3. As you can see all completes but the first test is passed down to the next report each time.

@YaroSpace
Copy link
Collaborator

Yes, I fixed that.

@hlidotbe
Copy link
Contributor Author

hlidotbe commented Jun 8, 2025

Sorry I missed your last message. I just tested your fix and it works as expected, thanks!

@YaroSpace YaroSpace merged commit 34c4c50 into mistweaverco:develop Jun 8, 2025
3 checks passed
YaroSpace added a commit that referenced this pull request Jun 8, 2025
…th_resolver to options (#577)

* fix(oauth): set expires_in for manually entered Authorization Code
* feat(scripts): resolve NODE_PATH to nearest `node_modules`, add
node_path_resolver to options.
* feat(exporter): export all variables
* feat(ui): allow overriding Kualala UI buf/win opts with `ui.win_opts`
* feat(lsp): graphQL autocomplete
* feat(scripting): execute inline/file scripts in the order of declaration
* feat(lsp): add default keymaps for distros that do not set them
* fix(cli): avoid race condition preventing reporting some queries (#581)
---------

Co-authored-by: Yaro <[email protected]>
Co-authored-by: Ben <[email protected]>
Co-authored-by: Hugues Lismonde <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants