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

Add unit tests to create and verify base url permutations. #2484

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

jamwil
Copy link
Contributor

@jamwil jamwil commented Apr 26, 2024

Unit tests to verify that a site::Site is successfully built and contains valid permalinks using the new base url logic in src/cmd/serve.rs introduced in #2311.

Closes #2448.

I am marking this as a draft for now. This tests the create_new_site function and verifies the urls and paths, but I'd like to add further tests of the actual live hyper endpoint.

Edit: Not sure the hyper endpoint tests are important at this stage.

Note that the tests will fail until this rebased on #2482 as that PR makes the trailing slash behaviour consistent with prior before we added the construct_base_url function.

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

@jamwil jamwil force-pushed the test-serve branch 2 times, most recently from 19dc8a1 to 551c2f9 Compare April 26, 2024 22:23
@jamwil jamwil changed the title Add unit tests to create a verify base url permutations. Add unit tests to create and verify base url permutations. Apr 26, 2024
@Keats
Copy link
Collaborator

Keats commented Jun 15, 2024

If you rebase is it ok to merge?

@jamwil
Copy link
Contributor Author

jamwil commented Jun 17, 2024

@Keats Hey Vincent, my apologies—was on a long trip, followed by a large backlog resulting from said long trip.

Yes, I think this is ok to merge—these tests are adequate for our purposes as outlined in #2448. I'll rebase shortly (today or tomorrow). Thanks.

@jamwil jamwil marked this pull request as ready for review June 17, 2024 16:45
@jamwil
Copy link
Contributor Author

jamwil commented Jun 17, 2024

@Keats Good to merge, mate. Cheers.

}

#[test]
fn test_create_new_site_with_protocol_without_port_with_mounted_path() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this fails on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the tests were green prior to rebase. The process cannot access the file because it is being used by another process. This seems like a concurrency issue with the test runner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we try re-running and see if it was transient?

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does fail very often on CI in the end, I'll disable it for windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that's unfortunate. I have a Windows VM I'll see if I can reproduce the failure outside of CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is peculiar if it only fails on the one test you disabled as there are several that are virtually identical, just using slightly different parameters.

I expect the failure to reemerge on the next active test of that type...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah it's weird. I've disabled it so i can release 0.19 but it would be interesting to get some investigation done

@Keats Keats merged commit a32b3b0 into getzola:next Jun 17, 2024
5 checks passed
@jamwil jamwil deleted the test-serve branch June 27, 2024 21:13
berdandy pushed a commit to berdandy/azola that referenced this pull request Sep 17, 2024
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.

2 participants