Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(server): Use global config from file if provided #2458
feat(server): Use global config from file if provided #2458
Changes from all commits
dbc71f3
a320a33
dc8fe7a
d631912
c125ec5
aed5e6e
153c9f1
9367f89
9068be4
1a43645
3776ec6
2d56213
761198e
c3221b7
8fa4521
387c246
0fadcea
749306a
5281b67
68b0bbf
383d850
67d3f73
43e4b4c
e9b7272
5e94b76
9e0f3e3
4f87968
da63e18
a5e9c03
a7030d4
a5c0224
a6efd91
99f2acd
20a593e
17eba98
a6ada9f
a69eb2f
a4f5a14
c0e767d
c9f0ef9
c39f12c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the static mode we actually never get this
fetch_handle
trigger? Is it right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right. The fetch handle is only scheduled when handling a request, and a request is only made in managed modes; thus, in static mode, this fetch handle is never resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of
#[should_panic]
, which is intended to be used for expected panics, we should rather make sure the upstream service returns a default config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, you suggest returning a response as if it were the actual upstream service. To actually return a global config we need to do an async operation in the closure, currently unstable in Rust. I think the
panic
makes the test easy to understand, and removes the complexity of dealing withmock_service
. Maybe I'm missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm here with @jan-auer . The thing is with unit tests, it can also panic for completely different reason when something changes in the code, and therefore the tests will be testing completely wrong thing.
It's better to make sure the behaviour is correct, but checking the status of some operation, like in this case that resulting config is
default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TBS1996 and I spent some time on this today. In order to move forward with this PR, let's keep the
should_panic
for now. He'll link a follow-up issue here to clean this up.We'll implement some utilities to make mocking upstream requests easier. The boilerplate needed to achieve this now for a single test is too complex.