search

pharo-contributions / XML-XMLParser

XML Parser for Pharo
MIT License
11 stars 17 forks source link

Some HTTP tests fail in Pharo 10 #8

Closed tinchodias closed 1 year ago

tinchodias commented 2 years ago

I don't understand why is being tested: Same key and value is added twice to a ZnMessage header, and the tests expect that the multiplicity is kept.

Maybe something changed in Pharo 10's Zinc? But anyway, I don't know why is this important at the level of a XML Parser. Probably some historical reason.

Any opinion from experts on this project?

tinchodias commented 2 years ago

(The tests don't fail on Pharo 9)

svenvc commented 1 year ago

I had a look at the failing tests. I can't immediately find a recent change in Zinc that would have caused this. Zinc has always supported multi valued headers and but not duplicate same key-value headers, but I might remember incorrectly.

IMHO the tests are wrong: both tests (testAddHeader and testHeaderValuesAtAdd run for 2 subclasses) take a message and add a bunch of headers (as defined in the constant methods headers), I understand the idea. Now the headers are added twice, once to a message where they are not yet present and once to a message where they are already present. But in the second loop the addedHeaders collection is extended again. That can never work.

Removing the second occurrence of "addedHeaders addLast: each." in both tests fixes the test, AFAIU.

svenvc commented 1 year ago

I was running this on Pharo 11 BTW

svenvc commented 1 year ago

PR for review: https://github.com/pharo-contributions/XML-XMLParser/pull/13

svenvc commented 1 year ago

So now this fails on Pharo 9.

It took me while to figure this out, but at the end April 2018, I did a patch that changed the behaviour of ZnHeaders (actually its underlying ZnMultiValueDictonary). Being able to add the same header/key twice is a key feature here. The change was that adding the same header/key with exactly the same value became idempotent (i.e. had no effect), while before it was added twice (and reported as being there twice).

This patch was done in response to a problem on the Seaside list where someone had trouble with double Connection:Close headers.

In any case I think that the latest changes are correct.

Problem is that all Pharo version prior to 10 contain the old code and will thus fail.

I also agree that it is a bit strange that the XML package has such deep HTTP tests, maybe there is a reason, I don't know.

I will try to modify the PR to skip the test on Pharo versions before 10.

That feels like a good compromise.

svenvc commented 1 year ago

The PR was merged, I guess that this issue can be closed

astares commented 1 year ago

Yes