Skip to content

Add testing helper for DELETE requests #136

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

Merged
merged 5 commits into from
Oct 13, 2021
Merged

Add testing helper for DELETE requests #136

merged 5 commits into from
Oct 13, 2021

Conversation

teisenbe
Copy link
Contributor

This introduces test_util.object_delete(), to go along with the existing test_util helpers. It's helpful for testing CRUD APIs.

This introduces test_util.object_delete(), to go along with the
existing test_util helpers.
@teisenbe teisenbe added the 🌟 enhancement New feature or request. label Oct 13, 2021
@teisenbe teisenbe requested a review from davepacheco October 13, 2021 01:05
@teisenbe
Copy link
Contributor Author

Hmm, those tests seem to be failing for me on main. Not sure what's going on there

@smklein
Copy link
Contributor

smklein commented Oct 13, 2021

Hmm, those tests seem to be failing for me on main. Not sure what's going on there

What's the error message you're seeing? I just did git checkout main && cargo test, not seeing anything

(Edit: I see it - in actions, though not locally. More detail below)

@smklein
Copy link
Contributor

smklein commented Oct 13, 2021

Hmm, those tests seem to be failing for me on main. Not sure what's going on there

What's the error message you're seeing? I just did git checkout main && cargo test, not seeing anything

Ahhhh taking a closer look here - and at https://github.com/oxidecomputer/dropshot/actions - I think the reason tests (unrelated to your PR) tests are failing is because:

  1. We have some tests that check for "compiler failures" looking like we want
  2. I suspect the compiler changed, and the compiler output changed
  3. This change - though probably innocent - caused "expected != actual", and the test failed.

Eyeballing it, it actually kinda looks like the only line that changed was actually part of the compiler message giving path hints:

Expected: ::: $WORKSPACE/dropshot/src/api_description.rs

vs

Actual: ::: src/api_description.rs:54:33

I'll follow-up with a separate issue.

@smklein
Copy link
Contributor

smklein commented Oct 13, 2021

Filed #138

@smklein smklein mentioned this pull request Oct 13, 2021
@teisenbe
Copy link
Contributor Author

Okay, should be ready for another review. Tests all pass locally after merging #139 in

@teisenbe
Copy link
Contributor Author

Hmm, the windows build failed logging::test::test_config_file with this output:

---- logging::test::test_config_file stdout ----
config "file": Ok(File { level: Warn, path: "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\dropshot-af77ac2c25c9d09a.exe.2092.file_dir\\log.out", if_exists: Fail })
note: configured to log to "C:\Users\RUNNER~1\AppData\Local\Temp\dropshot-af77ac2c25c9d09a.exe.2092.file_dir\log.out"
config "file": Ok(File { level: Warn, path: "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\dropshot-af77ac2c25c9d09a.exe.2092.file_dir\\log.out", if_exists: Append })
note: configured to log to "C:\Users\RUNNER~1\AppData\Local\Temp\dropshot-af77ac2c25c9d09a.exe.2092.file_dir\log.out"
config "file": Ok(File { level: Trace, path: "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\dropshot-af77ac2c25c9d09a.exe.2092.file_dir\\log.out", if_exists: Truncate })
note: configured to log to "C:\Users\RUNNER~1\AppData\Local\Temp\dropshot-af77ac2c25c9d09a.exe.2092.file_dir\log.out"
thread 'logging::test::test_config_file' panicked at 'assertion failed: should_be_before.timestamp() <= time_after.timestamp()', dropshot\src\test_util.rs:786:13

This seems unrelated to my change; I can open a ticket for this

@teisenbe
Copy link
Contributor Author

It passed on a re-run. I opened #141 to track

@teisenbe teisenbe merged commit 928da85 into main Oct 13, 2021
@teisenbe teisenbe deleted the add_object_delete branch October 13, 2021 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants