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

rework symlink test #445

Merged
merged 4 commits into from
Jan 28, 2024
Merged

rework symlink test #445

merged 4 commits into from
Jan 28, 2024

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Jan 26, 2024

  • rework existing symlink test so that it creates the link itself (and removes it) - in /tmp folder for duration of test
  • aim is to get 1.0.x ready for 1.0.1-RC2 - will forward fit after it is merged

#275, #444

@pjfanning pjfanning marked this pull request as draft January 26, 2024 19:14
@pjfanning pjfanning changed the title [DRAFT] rework symlink test rework symlink test Jan 26, 2024
@pjfanning pjfanning requested a review from mdedetrich January 26, 2024 21:41
@pjfanning pjfanning marked this pull request as ready for review January 26, 2024 21:41
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Nothing immediate jumps out at me but I would get @jrudolph to check just to make sure nothing is missed

lgtm


// need to serve from the src directory, when sbt copies the resource directory over to the
// target directory it will resolve symlinks in the process
val testRoot = new File("http-tests/src/test/resources")
Copy link
Member

Choose a reason for hiding this comment

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

Better with Files.path, and yes Winodws is using different separator

Copy link
Contributor

Choose a reason for hiding this comment

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

So TIL that Windows has symlinks but only for Windows 10/11. I don't know how this factors into everything, my initial inclination is to disable the test for Windows since this seems to only really be testing behaviour specific to Posix systems.

@He-Pin Can you confirm if the test passes on your machine (assuming all of the Windows related path separator issues have been solved)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is copied from an existing test. The test either works or fails on Windows - either way noone is complaining about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that Windows Java installs know how to apply the file path even if it doesn't use Windows idioms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@He-Pin could you try running sbt http-tests/test? It doesn't take that long compared to similar commands in main pekko repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@He-Pin I created #446 - is ok to say that you are not blocking this? I would like to get this merged so that I can do an RC2 for the Pekko HTTP 1.0.1 release.

@pjfanning pjfanning merged commit c231269 into apache:1.0.x Jan 28, 2024
10 checks passed
@pjfanning pjfanning deleted the rework-symlink-test branch January 28, 2024 11:41
pjfanning added a commit to pjfanning/incubator-pekko-http that referenced this pull request Jan 28, 2024
* rework symlink test

* add test that uses temp file symbolic link (still not fully working)

* Update FileAndResourceDirectivesSymlinkSpec.scala

* Update FileAndResourceDirectivesSymlinkSpec.scala
pjfanning added a commit that referenced this pull request Jan 28, 2024
* rework symlink test

* add test that uses temp file symbolic link (still not fully working)

* Update FileAndResourceDirectivesSymlinkSpec.scala

* Update FileAndResourceDirectivesSymlinkSpec.scala
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.

3 participants