test: cls variant of test-http-abort-stream-end

[HarshithaKP]

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

[mhdawson]

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

[HarshithaKP]

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

[mhdawson]

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

[HarshithaKP]

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

[HarshithaKP]

@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.

[mhdawson]

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

[HarshithaKP]

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

[addaleax]

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

[HarshithaKP]

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

[addaleax]

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]

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]

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

[HarshithaKP]

Moved test case to parallel folder.

[mhdawson]

@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]

@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]

@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]

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

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.