Skip to content

[4.2] Ubuntu18.04 fixes for TestURLSession #1615

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Aug 2, 2018
Merged

[4.2] Ubuntu18.04 fixes for TestURLSession #1615

merged 9 commits into from
Aug 2, 2018

Conversation

spevans
Copy link
Contributor

@spevans spevans commented Jun 22, 2018 •
edited
Loading

This is a back port of various fixes to allow TestURLSession to work on Ubuntu18.04 without intermittent errors. It includes the following (some required because of what later fixes are based on)

#1602
#1613
#1565
#1498
#1650

Tested on macOS, Ubuntu 16.04 and Ubuntu 18.04

@spevans
Copy link
Contributor Author

spevans commented Jun 22, 2018

@swift-ci please test

1 similar comment
@spevans
Copy link
Contributor Author

spevans commented Jun 24, 2018

@swift-ci please test

@alblue
Copy link
Contributor

alblue commented Jun 26, 2018

Looks like a test failure:

11:56:24 Test Case 'TestURLSession.test_httpRedirectionWithInCompleteRelativePath' started at 2018-06-24 16:56:01.307
11:56:24
TestFoundation/TestURLSession.swift:887: error: TestURLSession.test_httpRedirectionWithInCompleteRelativePath : XCTAssertEqual failed: ("200") is not equal to ("404") - HTTP response code is not 200

@shahmishal
Copy link
Member

@swift-ci please test

@shahmishal
Copy link
Member

@spevans Can you look into this test failure?

Test Case 'TestURLSession.test_httpRedirectionWithInCompleteRelativePath' started at 2018-08-01 21:05:45.164
TestFoundation/TestURLSession.swift:879: error: TestURLSession.test_httpRedirectionWithInCompleteRelativePath : XCTAssertEqual failed: ("200") is not equal to ("404") - HTTP response code is not 200
Test Case 'TestURLSession.test_httpRedirectionWithInCompleteRelativePath' failed (0.021 seconds)

spevans and others added 8 commits August 2, 2018 02:28
@spevans
SR-7620: Use charset encoding if available for NSString(contentsOf:us…
880732e 
…edEncoding:)
@spevans
NSCondition fixes: Ensure correct use of lock()/action/unlock() seque…
d7f4af7 
…nce (swiftlang#1613)
@spevans
DarwinCompatibility: HTTPCookie fixes for version 0 cookies
826a055 
- For expiry date, prefer maximum-age over expires-date but only
  use maximum-age for version 1 cookies.

- For version 0 cookies only return the first port number,
  if available.
@spevans
Test HTTPServer.swift fixes:
7f55f7b 
- Set SO_NOSIGPIPE on the client socket (Darwin) or
  use send() with flag MSG_NOSIGNAL (Linux).

- listen() just after bind()ing.

- Close the client socket after writing out the response.

- When sending a response with a start delay dont use asyncAfter()
  with a semaphore, just use Thread.sleep().

- Fix parsing the HTTP request so that all of the header lines are
  read correctly instead of reading the last header line as the body.

- When waiting for the server to become ready, wait for the semaphore
  with a timeout rather than an indefinite wait().

- Add setUp() to be run before each test class.

- For test cases that check the respnse body, check the data is not nil
  before reading it.

- Use 1 server for whole class clifetime.

- Use shutdown() to interrupt accept() when shutting down the server.
@spevans
TestURLSession fixes:
4b39b43 
- Uncomment failing tests that now work, fixes SR-7723 and SR-5751.

- Disable TestURLSession.test_cancelTask() as it fails intermittently
  because the server can respond before the task is cancelled.

- Disable TestURLSession.test_simpleUploadWithDelegate() as the server
  wont read the entire body successfully.
@spevans
TestURLSession: Fix test_cancelTask()
998a72b 
- Add a delay into the test server response so that the task will always
  be cancelled before the server returns the complete response.
@spevans
TestURLSession: Undo discretionary/isDiscretionary change
34060d6
@tid-kijyun @spevans
[SR-7112] Fixed a case where CFStringGetCStringPtr returns a string w…
b2c8092 
…ithout null termination

Changed _fastCStringContents (used in CFStringGetCStringPtr) to return NULL if required to return a string containing null termination.
Since this fix, if you call CFStringGetCStringPtr with a Swift-based CFString, it returns NULL.

Before the change: _fastCStringContents always returned a pointer to an ASCII string containing no NULL termination.
This is because the code was using unsafeBitCast to get internal pointer of String of Swift. Swift's string does not guarantee null termination.
@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

@shahmishal Im just rebasing and testing locally now, If I remember correctly this actually worked ok on my Ubuntu18 server so it may be an issue with Ubuntu16. Is it possible to use swift-ci to direct tests to the individual Ubuuntu18/16/14 servers? I don't want to get this working then find it breaks on Ubuntu14 after merging as is sometimes the case.

@spevans spevans force-pushed the pr_ubuntu18_fixes_42 branch from 53c06ef to b2c8092 Compare August 2, 2018 04:35
@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

@swift-ci please test

@shahmishal
Copy link
Member

@swift-ci test

@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

@swift-ci please test

1 similar comment
@shahmishal
Copy link
Member

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

At the moment this PR tests successfully on my local 16.04 and 18.04 test servers so it might take a while to figure out the issue on the CI server. Im just testing on 14.04 to see if that shows up the issue

@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

Looks like the issue is a bug in the redirection handling code. The test is failing because the redirect is not occurring correctly but it also looks like there is an Apache server running on the CI host which is intercepting these incorrect redirects and then serving a different error code to that expected by the tests. Im just working on a fix now

@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

The test passed with the fix https://ci.swift.org/view/Pull%20Request/job/swift-corelibs-foundation-PR-Linux/1190
I just need to create a PR for it and get it reviewed, merged into master first.

@spevans spevans mentioned this pull request Aug 2, 2018
Merged
@spevans spevans force-pushed the pr_ubuntu18_fixes_42 branch from ab31ed1 to b2c8092 Compare August 2, 2018 15:13
@spevans
When redirecting with a relative Location, add the port number to the…
a657b2e 
… new URL

- If the new location includes a host but no port number, then DONT add
  the port number.
@spevans spevans force-pushed the pr_ubuntu18_fixes_42 branch from 2d550d5 to a657b2e Compare August 2, 2018 15:31
@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

@swift-ci please test

1 similar comment
@shahmishal
Copy link
Member

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

@shahmishal Looks good now

@shahmishal shahmishal requested a review from parkera August 2, 2018 20:59
@spevans
Copy link
Contributor Author

spevans commented Aug 2, 2018

@swift-ci please test and merge

@shahmishal shahmishal merged commit 4c63b3e into swiftlang:swift-4.2-branch Aug 2, 2018
@maksimorlovich maksimorlovich mentioned this pull request Aug 27, 2018
Merged
This was referenced Aug 3, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera parkera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@spevans @alblue @shahmishal @parkera @tid-kijyun