NC | Online Upgrade | Tests | Config directory restructure upgrade script unit tests #8654
Conversation
romayalon
force-pushed
the
romy-config-dir-restructure-tests
branch
3 times, most recently
from
513d2bd to
f321d16
Compare
Signed-off-by: Romy <35330373+romayalon@users.noreply.github.com>
romayalon
force-pushed
the
romy-config-dir-restructure-tests
branch
from
f321d16 to
2ea7b60
Compare
| async function folder_delete_skip_enoent(dir) { | ||
| if (!dir) return; | ||
| try { | ||
| return rimraf(dir); |
There was a problem hiding this comment.
I think it would be easier for readability to reuse the function in this file:
| return rimraf(dir); | |
| return folder_delete(dir); |
| @@ -191,6 +191,15 @@ function folder_delete(dir) { | |||
| return rimraf(dir); | |||
| } | |||
|
|
|||
| async function folder_delete_skip_enoent(dir) { | |||
There was a problem hiding this comment.
If you want you can JSDoc '9I understand that most of this file don't have it...).
| * @returns {Promise<Void>} | ||
| */ | ||
| async function write_manual_config_file(type, config_fs, config_data, invalid_str = '') { | ||
| async function write_manual_config_file(type, config_fs, config_data, invalid_str = '', { symlink_name, symlink_access_key} = {symlink_name: true, symlink_access_key: true}) { |
There was a problem hiding this comment.
If I understand correctly, in this approach you used default values for the whole object, so only if someone doesn't pass the options argument would it get those defaults.
Otherwise, if one passes one of them, it would get only the one that he passed.
Is that what you meant?
| * @param {Object} config_data | ||
| * @returns {Promise<Void>} | ||
| */ | ||
| async function create_identity_dir_if_missing(config_fs, config_data) { |
There was a problem hiding this comment.
You can pass only the ID as an argument (instead of the config_data object).
| if (type === CONFIG_TYPES.ACCOUNT && symlink_access_key && config_data.access_keys) { | ||
| await symlink_account_access_key(config_fs, config_data.access_keys[0].access_key, id_relative_path); |
There was a problem hiding this comment.
- I'm not sure it is enough
&& config_data.access_keys- anonymous account would have access_key as [] (defined, but empty array). - If you can change it from the first item (
config_data.access_keys[0]) to iterate on the array ofaccess_keys, as this is a helper function and we might have more than 1 access key.
|
|
||
| it('create_identity_if_missing() - only dir exists but file doesn’t', async () => { | ||
| const new_account_data = { _id: String(1), name: 'old_account' + 1, user: 'root', access_keys: [{ access_key: mock_access_key }] }; | ||
| await write_manual_old_account_config_file(default_config_fs, new_account_data); |
There was a problem hiding this comment.
Could you add a comment in the line where we used to create the config file in the past but didn't so it will clear?
| // Jest has builtin function fail that based on Jasmine | ||
| // in case Jasmine would get removed from jest, created this one | ||
| // based on this: https://stackoverflow.com/a/55526098/16571658 | ||
| function fail(reason) { | ||
| throw new Error(reason); | ||
| } |
There was a problem hiding this comment.
If you want we can move it to a new file "jest_utils" (not necessarily in this PR).
| } | ||
| }); | ||
|
|
||
| it('prepare_account_upgrade_params() - account name exists', async () => { |
There was a problem hiding this comment.
Are you sure about the test title here? (in the previous test, we had the same title.
| await assert_account_config_file_upgraded({[new_account_data.name]: new_account_data}); | ||
| }); | ||
|
|
||
| it('upgrade_account_config_file() - identity exists but indexes are not', async () => { |
There was a problem hiding this comment.
What do you mean by "identity exists but indexes are not"? (the term "indexes" is links?)
Could you write the title with what explicitly is missing (access key link, identity link, etc.).
| await assert_account_config_file_upgraded({[new_account_data.name]: new_account_data}); | ||
| }); | ||
|
|
||
| it('upgrade_account_config_file() - identity exists, name index exist, access keys index does not', async () => { |
There was a problem hiding this comment.
Again, why index?
| it('upgrade_account_config_file() - identity exists, name index exist, access keys index does not', async () => { | |
| it('upgrade_account_config_file() - identity exists, name symlink exist, access keys symlink does not', async () => { |
|
@romayalon could you update the doc file of CI&Tests.md with the new tests files? |
Explain the changes
noobaa-core/src/upgrade/nc_upgrade_scripts/1.0.0/config_dir_restructure.js./etc/noobaa.conf.d/accounts_by_name/account1.symlinkpoints to a wrong/non existing location and then re-creating the link correctly, added also the matching tests for this case - notice this is an artificial test and is not suppose to happen in the usual flow.The relevant tests for this case -
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
sudo jest --testRegex=jest_tests/test_config_dir_restructure_upgrade_script.test.js