Skip to content

Conversation

addaleax
Copy link
Collaborator

Includes #669 because it’s needed here.

Add a test that verifies the actual API key bundled into the actual
release binary. In order to achieve this, add an internal test command
that performs a Segment API request and verifies that its callback
does not return an error.

Note that this is, currently, a test that with somewhat limited
meaningfullness, because the Segment API currently always gives
successful HTTP responses, regardless of the validitity of the API key.

(As such, I would also consider it okay to not merge this. It still tests
that we can reach the Segment API endpoint, though, i.e. the
analytics-node package is properly configured.)

This otherwise messes with cases where the input stream ends while
async evaluation is still in progress (e.g. when piping into mongosh).
Add a test that verifies the actual API key bundled into the actual
release binary. In order to achieve this, add an internal test command
that performs a Segment API request and verifies that its callback
does not return an error.

Note that this is, currently, a test that with somewhat limited
meaningfullness, because the Segment API currently always gives
successful HTTP responses, regardless of the validitity of the API key.
@mmarcon
Copy link
Member

mmarcon commented Feb 18, 2021

the Segment API currently always gives
successful HTTP responses, regardless of the validitity of the API key

😮

Copy link
Contributor

@rose-m rose-m left a comment

Choose a reason for hiding this comment

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

That's where your tweet came from 😂

Like the solution that it really test the analytics in a "smoke" fashion 👍

On the other hand I have the tiny feeling that we are starting to make more and more exceptions and "tiny balcony" options here and there for tests, etc. Maybe I'm also just overreacting here but I just want to ensure we're not going down exception hell already 😅
If I understood the ticket correctly it would maybe be sufficient to "just" run an API call from evergreen as we have other tests to verify that the analytics are properly generated/included [well, if we could see from the API call if it works at all].

@addaleax
Copy link
Collaborator Author

On the other hand I have the tiny feeling that we are starting to make more and more exceptions and "tiny balcony" options here and there for tests, etc. Maybe I'm also just overreacting here but I just want to ensure we're not going down exception hell already sweat_smile

I don’t really think we’re running into that problem – it’s still just a tiny fraction of our overall tests that makes this up.

If I understood the ticket correctly it would maybe be sufficient to "just" run an API call from evergreen as we have other tests to verify that the analytics are properly generated/included [well, if we could see from the API call if it works at all].

Well … we have tests that verify that, and we have tests that verify that we can use analytics-node using a fake HTTP server – if we’re not testing that the actual functionality from the shell works, then I’m not sure that there’s a point in testing this at all :)

@rose-m
Copy link
Contributor

rose-m commented Feb 18, 2021

I understood it as such that the API key needs to be verified to be valid as we might generate a new one in the future / use a wrong expansion / etc. So that's what a "simple" GET at the right place would verify. Of course the smoke test solution verifies the whole chain 👍

@addaleax
Copy link
Collaborator Author

Right, but as I mentioned, this currently doesn’t work with the segment API. Should we just not do this?

@rose-m
Copy link
Contributor

rose-m commented Feb 18, 2021

I'd leave it out for now, yes. So 🤦 that their API doesn't allow to check the keys...

@addaleax
Copy link
Collaborator Author

Cool, I'll close this as wontfix then. We can always revisit if something changes in their API :)

@addaleax addaleax closed this Feb 19, 2021
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