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.

Sign up for GitHub

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

Jump to bottom

fix: correcting teardown for integ test and code cleanup #188

Merged
merged 1 commit into from
Feb 8, 2024
Merged

fix: correcting teardown for integ test and code cleanup #188

merged 1 commit into from
Feb 8, 2024

Conversation

ts-amz
Copy link
Contributor

@ts-amz ts-amz commented Feb 8, 2024

Description

  • Main change is to add to the device-patcher teardowns to delete the patches created in the course of the tests. There isn't a straightforward way to delete the tasks as well, but so far, those don't seem to be causing errors. We can revisit later.
  • re-enabled patch.feature test because correcting the teardown should ensure that it works consistently
  • config-inject added to the logger because it's required for the config values to be loaded before logger creation (log level is in configs), also reordered some imports because of this
  • updated the logger to warn if the configuration hasn't been set when it's created (though that should be impossible now)
  • added more specific error logging and cleaned up some typos
  • updated to vscode new value for organizingImports

Type of change

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (existing code being refactored)
  • This change includes a documentation update

Submission Checklist

  • CI dry-run passing
  • [x ] Integration tests passing locally
  • Change logs generated
Additional Notes:

aalaabed
aalaabed reviewed Feb 8, 2024
View reviewed changes
source/packages/integration-tests/src/step_definitions/device-patcher/deployment.step.ts Outdated
@@ -295,6 +294,7 @@ Then(
patchTask,
getAdditionalHeaders(world.authToken)
);
// console.log('patchTaskId: ' + this['patchTaskId']);
Copy link

Choose a reason for hiding this comment

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

Don't we need to remove 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.

I kept these because it's annoying to re-add when you're debugging, but I didn't want them there by default. They're in the tests, so I don't think they hurt anything. Can remove if you feel strongly about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want them while debugging, would it make more sense to uncomment them and use console.debug?

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 changed it to use the logger (I'm not sure what's setting the console.log log level, but it's not working as expected). This consolidates in on the logger anyway, and we set that log level in the integ test script.

source/packages/integration-tests/src/support/device_patcher_hooks.ts Outdated

// delete all patches for the given device
const deletionPromises = patches.map((patch): Promise<void> => {
console.log(`deleting: ${patch.patchId}`);
Copy link

Choose a reason for hiding this comment

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

Is there a reason to use console.log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are in the integ tests, so I don't think it matters either way, but I removed it.

Copy link

Choose a reason for hiding this comment

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

I'd remove them, but this is a NIT.

source/packages/integration-tests/src/support/greengrass2_provisioning_hooks.ts Outdated
@@ -29,7 +29,7 @@ import { container } from '../di/inversify.config';
import { AUTHORIZATION_TOKEN } from '../step_definitions/common/common.steps';
import { getAdditionalHeaders } from '../step_definitions/notifications/notifications.utils';

import { logger } from '@awssolutions/simple-cdf-logger';
// import { logger } from '@awssolutions/simple-cdf-logger';
Copy link

Choose a reason for hiding this comment

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

can we remove this as well?

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 kept these because it's annoying to re-add when you're debugging, but I didn't want them there by default. They're in the tests, so I don't think they hurt anything. Can remove if you feel strongly about it.

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 turned these back on and set them to the silly log level so they don't show in our current default log level.

@ts-amz ts-amz requested a review from pcozzi February 8, 2024 18:49
@ts-amz ts-amz merged commit f0a0162 into aws:main Feb 8, 2024
1 check passed
@ts-amz ts-amz deleted the chore_cleanup branch February 8, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalaabed aalaabed aalaabed approved these changes

@pcozzi pcozzi pcozzi approved these changes

@nrusson nrusson Awaiting requested review from nrusson

@BenjiTheC BenjiTheC Awaiting requested review from BenjiTheC

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ts-amz @pcozzi @aalaabed