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 fixes for RedHat #1562

Merged
merged 2 commits into from May 26, 2018
Merged

Test fixes for RedHat #1562

merged 2 commits into from May 26, 2018

Conversation

spevans
Copy link
Collaborator

@spevans spevans commented May 24, 2018

  • Process: Make xdgTestHelper into a self-contained file

  • Build as part of DarwinCompatibilityTests.

  • Add TestBundle.xdgTestHelperURL() to return the URL of the
    executable.

  • Process: When setting the current directory for a sub-process,
    set the PWD environment variable to this directory if the
    default environment is used. This matches Darwin's behaviour.

  • Use xdgTestHelper as an external test program for TestProcess,
    taking options for getcwd() and getenv("PWD").

  • SR-7748: Use /tmp as a test directory and run it through realpath()
    to test against the result of getcwd(). On macOS, /tmp is a symlink
    to private/tmp.

  • SR-7747: FileManager.attributesOfFileSystem(forPath:) may return
    .systemNumber as 0 on some systems (eg tmpfs on Redhat). Remove
    this invalid test.

@spevans
Copy link
Collaborator Author

spevans commented May 24, 2018

@swift-ci please test

@davezarzycki
Copy link
Collaborator

  1. I think test_current_working_directory() is trying to verify that the caller's current working directory doesn't change when running a task in a different working directory. Therefore, changing the task's working directory to the root directory is at greater risk of being a "no op" test. I'd just drop the "pwd" assert and add a comment to the test about what the goal of the test is.
  2. I'd also add a comment to the code that initializes systemNumber to note that tmpfs on Linux can return zero.

@spevans
Copy link
Collaborator Author

spevans commented May 24, 2018

test_current_working_directory() is actually testing both the child process uses .currentDirectoryPath correctly and that the parent process retains its working directory. Although some extra testing has exposed a possible issue where PWD should be set in the environment so I will fix this and update the tests.

For systemNumber I don't think a comment is actually required, the test was based on the wrong assumption that it could never be 0.

@davezarzycki
Copy link
Collaborator

Seems reasonable

@spevans
Copy link
Collaborator Author

spevans commented May 25, 2018

@swift-ci please test

let exeName = "/xdgTestHelper.app/Contents/MacOS/xdgTestHelper"
#else
let exeName = "/xdgTestHelper/xdgTestHelper"
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ok, but it's somewhat ironic that bundle can't find this helper itself. That's what it's supposed to be there fore in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that we are not building xdgTestHelper in a bundle directory structure. It may be worth it to figure that out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll aim to do that in a follow-up PR as it is complicated by the fact that there are 3 separate targets being built (Linux, macOS and the Darwin Compatibility testing). However it should act as a good test of the Bundle class so it is definitely worth doing.

@spevans
Copy link
Collaborator Author

spevans commented May 25, 2018

@davezarzycki Are you able to give this a test on redhat?

@millenomi
Copy link
Contributor

@swift-ci please test

@davezarzycki
Copy link
Collaborator

I haven't tested them, but they look fine.

- Build as part of DarwinCompatibilityTests.

- Add TestBundle.xdgTestHelperURL() to return the URL of the
  executable.
- Process: When setting the current directory for a sub-process,
  set the PWD environment variable to this directory if the
  default environment is used. This matches Darwin's behaviour.

- Use xdgTestHelper as an external test program for TestProcess,
  taking options for getcwd() and getenv("PWD").

- SR-7748: Use /tmp as a test directory and run it through realpath()
  to test against the result of getcwd(). On macOS, /tmp is a symlink
  to private/tmp.

- SR-7747: FileManager.attributesOfFileSystem(forPath:) may return
  .systemNumber as 0 on some systems (eg tmpfs on Redhat). Remove
  this invalid test.
@spevans
Copy link
Collaborator Author

spevans commented May 26, 2018

@swift-ci please test and merge

1 similar comment
@spevans
Copy link
Collaborator Author

spevans commented May 26, 2018

@swift-ci please test and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants