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: cls variant of test-http-abort-stream-end #32084

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

HarshithaKP
Copy link
Member

Refs: #31978

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 4, 2020
@mhdawson
Copy link
Member

mhdawson commented Mar 6, 2020

We should probably agree on whether we should use als or cls for the naming.

@HarshithaKP
Copy link
Member Author

@mhdawson, cls and als are used interchangeably in test cases. Confused which one to use. Open for suggestions.

@mhdawson
Copy link
Member

mhdawson commented Mar 9, 2020

Are there other test cases with cls in the name? If not I'd probably go with als

@HarshithaKP
Copy link
Member Author

@mhdawson, thanks. Other test cases are named with als. I will change the name to als.

@HarshithaKP
Copy link
Member Author

HarshithaKP commented Mar 11, 2020

@mhdawson, Original test cases coming along with API are named async-local-storage. In my case, I am migrating existing API scenarios to use CLS, so I need to retain the original names, and then append a word to indicate it is a cls variant. If I change the converted test case name similar to API test cases, the name becomes too long. Hence kept the name as it is by adding cls towards the end.
please let me know your views .
I Changed variable names to match the standard.

fixup: address review comment
@mhdawson
Copy link
Member

I think I'd still use als instead of cls unless there is some previous use of cls.

@HarshithaKP
Copy link
Member Author

@mhdawson, Changed the name to als, as there was no previous use of cls.

@addaleax
Copy link
Member

@HarshithaKP Did you mean to put the test file into another directly, e.g. parallel?

@HarshithaKP
Copy link
Member Author

@addaleax - I meant experimental folder itself, based on the suggestion at #31978 (comment)

@addaleax
Copy link
Member

Hm, okay … I don’t quite see the point of having tests that are allowed to fail, but in that case I would

  1. not name the folder experimental, that’s misleading if it’s not about experimental features, and
  2. make sure that the test runner actually allows these to fail.

But again, if something’s allowed to fail, then imo let’s just put it in parallel/ and mark it as flaky if an issue with it comes up.

@mhdawson
Copy link
Member

mhdawson commented Mar 13, 2020

I also do not think putting them into experimental makes sense. If they are allowed to fail I don't see the value unless it is temporary and in that case we should just wait until they do pass before adding them.

@mhdawson
Copy link
Member

Sorry last comment should have said do NOT think. Updated to reflect that

@HarshithaKP
Copy link
Member Author

Moved test case to parallel folder.

@mhdawson
Copy link
Member

@HarshithaKP, @gireeshpunathil I went to look at the original test. I had thought the idea was to find tests that had needed/used something similar to als as part of the http tests and then make versions which used als as an additional more real-world test of als.

I'm not really sure this matches that case as it seems to use globals which work just fine in this case. Can you add some details as why this was one of the tests that was a good fit?

@HarshithaKP
Copy link
Member Author

@mhdawson, This test does not use any new globals. The original test was making use of a data handler as a closure, which accumulated incoming data into a buffer, aborted the request half way, and made assertions on the aborted state and the data size. we converted data handler to a non-closure function and used als to store the data length instead.

So I guess this is a good fit in terms of usage of als to store contextual data (incoming data size in this case) rather than relying on passing around, or holding up in a closure.

@mhdawson
Copy link
Member

@HarshithaKP I'm not sure replacing the use of closures it was I expected/thought made sense. Maybe we can discuss in more in depth in the next Diagnostics WG meeting.

@aduh95
Copy link
Contributor

aduh95 commented Sep 19, 2023

@HarshithaKP do you still want to merge this? If so, can you please rebase on top of main?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants