Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Add Logging level tests (and vararg version of logCallingFunction) #373

Merged
merged 3 commits into from
Aug 14, 2020

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Aug 13, 2020

This PR adds unit tests as a part of: #370

See gl-native for the associated fix for #370 (which uses Log::Observer instead of calling down to -[MGLNetworkConfiguration debugLog:] etc).

Fixes #347 (along with next gl-native release)
Fixes mapbox/mapbox-gl-native#16511

@julianrex julianrex requested a review from 1ec5 as a code owner August 13, 2020 02:02
@julianrex julianrex requested a review from a team August 13, 2020 02:02
@julianrex julianrex self-assigned this Aug 13, 2020
@julianrex julianrex removed request for a team and 1ec5 August 13, 2020 02:02
Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

LGTM. But I left a question and requires commented code cleanup.

MGLLogDebug(format);
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-implementations"
// TODO: Remove in future release.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To bypass a catch-22; I marked the protocol methods as deprecated, and so the compiler warns (errors) if you add them. But if they're missing then the compiler errors about an incomplete conformance. 🤕

- (void)testLoggingLevelInfo {
[self internalTestLoggingLevel:MGLLoggingLevelInfo
expectingLogForLevels:@[
// @(MGLLoggingLevelDebug),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please remove commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tempted to leave for the moment, as in this case it's kinda documentation when seen alongside the other tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MGLNetworkConfiguration#errorLog drops error details MGLNativeNetworkManager#errorLog drops error details
2 participants