Skip to content

Conversation

@woodie
Copy link
Contributor

@woodie woodie commented Feb 10, 2025

The specs on the mock data were not doing what we'd expect, so I use the fixture data and make clear when we're testing a real payload. It's much easier to work with JSON as a separate file. I format the JSON with jq for readability.

It may be better to have Alert.WeatherAlert instead of WeatherAlert.Summary given that Summary is not the best way to describe everything.

In this PR we also stifle errors from a bad PKEY and allow the specs to run without a .env file.

@jameswpierce jameswpierce self-requested a review February 11, 2025 14:54
Copy link
Contributor

@jameswpierce jameswpierce left a comment

Choose a reason for hiding this comment

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

This LGTM. Approved with the caveat (that you are aware of) that that private key needs to be removed and decommissioned. Thanks for getting this in there, support for this was spotty when the API first came out.

@woodie woodie force-pushed the weather_alert_with_container branch from b876d86 to 33d90d6 Compare February 11, 2025 21:54
@woodie
Copy link
Contributor Author

woodie commented Feb 11, 2025

This LGTM. Approved with the caveat (that you are aware of) that that private key needs to be removed and decommissioned. Thanks for getting this in there, support for this was spotty when the API first came out.

OK, the PR is ready. Just for clarification, the PKEY I used was already decommissioned. I assume it must be a properly generated PKEY, but there is no need for it to be active.

We now simply allow(client).to receive(:token) to avoid this issue, but it will be a landmine that future developers could step on. I've also added a fallback so the specs can run without .env or anything in the ENV. With WebMock in place, it's not clear (to me) why we'd even want to load real credentials. @jameswpierce @seeReadCode

@woodie woodie force-pushed the weather_alert_with_container branch 5 times, most recently from 83007b7 to d851315 Compare February 12, 2025 16:54
@woodie woodie force-pushed the weather_alert_with_container branch from d851315 to a599903 Compare February 12, 2025 16:57
@seeReadCode
Copy link
Contributor

Thanks, I'll give it a spin in the next day or two so we can get this merged and released

@woodie woodie force-pushed the weather_alert_with_container branch from 820d59b to ab06731 Compare February 12, 2025 19:04
@drvn-mr
Copy link
Contributor

drvn-mr commented Feb 12, 2025

We now simply allow(client).to receive(:token) to avoid this issue, but it will be a landmine that future developers could step on.

This sounds like the sane thing to do here. Mocking isn't really a problem.
Tests shouldn't be sending outgoing requests or contain sensitive info.

@seeReadCode seeReadCode merged commit f8aae44 into superbasicxyz:main Feb 13, 2025
5 checks passed
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.

4 participants