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

[Identity] Different approach for testing the Arc feature, instead of mock-fs #15571

Merged
7 commits merged into from
Jun 5, 2021

Conversation

sadasant
Copy link
Contributor

@sadasant sadasant commented Jun 4, 2021

Recently we found an issue on our CI build for Node 16.3.0, which boiled down to one unit test specific to Identity: #15547

After some investigation, it is clear that Node 16.3.0 has some incompatibility with mock-fs: tschaub/mock-fs#332

In the mean time, to add this test again, we can simply create a file, point to it and delete it at the end. This should work in all versions of Node, and it means one less dev-dependency for us.

@sadasant sadasant requested a review from mikeharder June 4, 2021 21:46
@sadasant sadasant requested a review from schaabs as a code owner June 4, 2021 21:46
@sadasant sadasant self-assigned this Jun 4, 2021
@ghost ghost added the Azure.Identity label Jun 4, 2021
Un-did the rush update --full changes.

Used the temp directory
@sadasant
Copy link
Contributor Author

sadasant commented Jun 4, 2021

@mikeharder @praveenkuttappan I un-did the changes to the pnpm-lock file, after what Mike said

@sadasant sadasant requested a review from mikeharder June 4, 2021 22:45
// Trigger Azure Arc behavior by setting environment variables

process.env.IMDS_ENDPOINT = "https://endpoint";
process.env.IDENTITY_ENDPOINT = "https://endpoint";

const filePath = "path/to/file";
const filePath = join(tmpdir(), `${Date.now()}`);
Copy link
Member

Choose a reason for hiding this comment

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

I still think this has two issues:

  1. The filePath could collide if another test used Date.now() and ran at the same time.
  2. The file is not guaranteed to be deleted if an exception is thrown.

Here's my suggestion:

const fs = require('fs-extra');
const os = require('os');
const path = require('path');

const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "test-"));
try {
    const tempFile = path.join(tempDir, "test");
    fs.writeFileSync(tempFile, "hello");

    // Use tempFile
}
finally {
    fs.rmdirSync(tempDir, { recursive: true });
}

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’ll take your suggestion, but I don’t see the point of the folder! I’ll push something that should be enough though, unless I’m missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about this? e43377d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, one more improvement here: a11c1df

Copy link
Member

Choose a reason for hiding this comment

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

The point of the folder is Node provides an API for creating a guaranteed-unique temp folder (fs.mkdtemp()) but not a guaranteed-unique temp file.

This code is still vulnerable to possible collisions, but if you think it's good enough for our test code then I won't block 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.

ohh I see your point! Thank you for pointing that out to me. I’ll follow your approach!

Copy link
Contributor Author

@sadasant sadasant Jun 4, 2021

Choose a reason for hiding this comment

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

@mikeharder I couldn’t use the recursive parameter because it wasn’t recognized by our typescript, so I deleted the file and then the directory: e82fe6a

@ghost
Copy link

ghost commented Jun 5, 2021

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 54c6689 into Azure:master Jun 5, 2021
@sadasant sadasant deleted the identity/add-test-again branch June 7, 2021 15:22
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Aug 10, 2021
track2 modify operationalinsights readme.go.md (Azure#15571)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants