[Test] Remove unit test case in zip.test.js

[ananzh]

Description

With existing fixture, test verify consistency of modes of files inside zip.test.js fails. This is due to diff file mode in different system when doing same command or operation. For example, in our ubuntu, when create a file using mkdir cmd, the file mode is 775. But local machine, the file mode is 755.

local env: drwxr-xr-x  62 ananzh  staff   1.9K Jun 23 19:12 OpenSearch-Dashboards (755)
Ubuntu: drwxrwxr-x 24 anan anan 4096 Jun 24 01:04 OpenSearch-Dashboards (775)

After unzip executable has mode 777 and not-executable has mode 644. These two numbers are fixed. They are not dependent on diff system or folder mode where you unzip the files. However, the following two results:

getMode(path.resolve(tempPath,'executable'))
getMode(path.resolve(tempPath,’not-executable'))

are dependent on tempPath mode because it will use the lower mode. For example (use mode# directly below):
1)If tempPath is 775 then:

getMode(path.resolve(775, 777)) —> 775
getMode(path.resolve(775, 644)) —> 644

2)If tempPath is 755 then:

getMode(path.resolve(755, 777)) —> 755
getMode(path.resolve(755, 644)) —> 644

To verify by yourself, you could change getMode function and for example pass it a 755:
getMode = (path) => (fs.statSync(path).mode & parseInt('755’, 8)).toString(8);
Then in the test case, console log the mode of tempPath and test result:

console.log(getMode(tempPath));
console.log(getMode(path.resolve(tempPath, 'executable')));

Therefore, this unit test case result is dependent on the mode of tempPath which is affected by your system or system settings.

This PR investigats the issue and remove todo. We have two options: 1) Our Jenkins test env has 755 for tempPath (see below image). So we could enable this test case without any changes. Customer might fail this test locally and can manually skip this test.

Option2: Force the tempPath mode to be 755 by changing:

const getMode = (path) => (fs.statSync(path).mode & parseInt('755', 8)).toString(8);

However, this assumes customer's env will generate a tempPath either 755 or 775, which does not guaranteed that this test case won't fail again locally.

Option3: Remove this test case and open a new issue to investigate alternative ways to write this test case

This PR uses option 3. The meaning of this test case is to test whether extractArchive function extract files with same
permission when they archive. However, getMode function in this test suite is system env dependent which doesn’t serve the purpose of this test case.

Signed-off-by: Anan Zhuang [email protected]

Issues Resolved

#4

Test results

unit test for zip.test.js

yarn test:jest /home/anan/work/OpenSearch-Dashboards/src/cli_plugin/install/zip.test.js
yarn run v1.22.5
$ node scripts/jest /home/anan/work/OpenSearch-Dashboards/src/cli_plugin/install/zip.test.js
 PASS  src/cli_plugin/install/zip.test.js
  opensearchDashboards cli
    zip
      ✓ handles a corrupt zip archive (2 ms)
      analyzeArchive
        ✓ returns array of plugins (26 ms)
      extractArchive
        ✓ extracts files using the extractPath filter (10 ms)

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total
Snapshots:   3 passed, 3 total
Time:        1.884 s

Overall test result:

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

[opensearch-ci-bot]

✅   DCO Check Passed 86aa36b

[opensearch-ci-bot]

✅   DCO Check Passed 3112862

[tmarkley]

Maybe we should be questioning the actual expectations of the test. If we merely want to ensure that one file is executable and another isn't for everyone, we don't need to match exact file permissions. For example, should we also accept write and execute permissions with 333?

I think we should make the name of the test less vague and refactor so that it's not environment specific.

[ananzh]

Maybe we should be questioning the actual expectations of the test. If we merely want to ensure that one file is executable and another isn't for everyone, we don't need to match exact file permissions. For example, should we also accept write and execute permissions with 333?

I think we should make the name of the test less vague and refactor so that it's not environment specific.

After investigation and discussion, I suggest to remove this specific test case. getMode function is system env dependent. Remove path.resolve won't work:

expect(getMode(tempPath+'/executable')).toEqual('755');
expect(getMode(tempPath+'/not-executable')).toEqual('644'); 

The above test has same result.

The meaning of this test case is to test whether extractArchive function extract files with same permission when they archive:

expect(getMode('/executable')).toEqual('777');
expect(getMode('/not-executable')).toEqual('644'); 

It should not dependent on system env. Under this test suite, the getMode function can not test this and there is no other easy way to test mode in this test case. I will remove the test case for now and will open an issue.

[opensearch-ci-bot]

✅   DCO Check Passed dd1bf77

[ananzh]

Follow up issue: #561

[tmarkley]

Can you update the title and description since the test is now being removed?

[kavilla]

When squashing please make sure the commit message references that we just removed this test, otherwise if it gets merged with the titled it will be confusing when looking at the history.

[opensearch-ci-bot]

✅   DCO Check Passed 304372d

[ananzh]

Can you update the title and description since the test is now being removed?

Done. Thanks for reminding me.

[tmarkley]

LGTM, thanks for updating the details!