Skip to content
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

Decouple from TelemetryUTLogger #16466

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

anthony-murphy
Copy link
Contributor

TelemetryUTLogger is deprecated. This change moves existing usages to the MockLogger.

@anthony-murphy anthony-murphy requested review from a team as code owners July 20, 2023 00:28
@github-actions github-actions bot added area: driver Driver related issues area: loader Loader related issues area: odsp-driver area: runtime Runtime related issues labels Jul 20, 2023
@github-actions github-actions bot added the base: main PRs targeted against main branch label Jul 20, 2023
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jul 20, 2023

@fluid-example/bundle-size-tests: +3.54 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 455.11 KB 455.78 KB +686 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 244.54 KB 245.12 KB +592 Bytes
loader.js 151.23 KB 151.62 KB +399 Bytes
map.js 47.19 KB 47.45 KB +267 Bytes
matrix.js 147.86 KB 148.19 KB +336 Bytes
odspDriver.js 93.68 KB 94 KB +328 Bytes
odspPrefetchSnapshot.js 44.51 KB 44.79 KB +283 Bytes
sharedString.js 164.77 KB 165.1 KB +332 Bytes
sharedTree2.js 243.4 KB 243.79 KB +395 Bytes
Total Size 1.71 MB 1.71 MB +3.54 KB

Baseline commit: 65c8073

Generated by 🚫 dangerJS against dd33100

Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

🚀. Doesn't have to be in this PR, but this makes me want to add an .assertNoErrors() API to MockLogger.

deltaStorageService = new OdspDeltaStorageService(
testDeltaStorageUrl,
async (_refresh) => "",
createUtEpochTracker(fileEntry, logger),
logger,
);
});
afterEach(() => {
logger.assertMatchNone([{ category: "error" }]);
logger.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

All the methods that check for some condition on the events in the MockLogger already clear it so this line is not necessary. Not the most intuitive, IMO, but we do have tests that check for events more than once that rely on that behavior today.

I wouldn't be opposed to eventually changing that and making an explicit .clear() necessary, so I'm fine if we want to keep it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i removed the clears. i think the current behavior is fine. What i really want is to get the mock logger out of the prod package and put it into a test package somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally on board with that. If you get to that point soon, once it's not public API surface anymore I'll follow up with these changes.

@anthony-murphy anthony-murphy merged commit aebd5ed into microsoft:main Jul 20, 2023
27 checks passed
@anthony-murphy anthony-murphy deleted the decouple-utlogger branch July 20, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: driver Driver related issues area: loader Loader related issues area: odsp-driver area: runtime Runtime related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants