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

exclude homeassistant entries from null cleanup #22995

Merged
merged 4 commits into from
Jun 15, 2024

Conversation

ghoz
Copy link
Contributor

@ghoz ghoz commented Jun 9, 2024

skip homeassistant keys from the null values cleanup

Quick and sligtly dirty fix for #22976

lib/util/utils.ts Outdated Show resolved Hide resolved
@ghoz
Copy link
Contributor Author

ghoz commented Jun 12, 2024

updated to be less dirty.

lib/util/settings.ts Outdated Show resolved Hide resolved
@Koenkk
Copy link
Owner

Koenkk commented Jun 12, 2024

It seems the coverage fails, can you run npm run test-with-coverage and check whats missing in the report? (coverage/lcov-report/index.html)

@ghoz
Copy link
Contributor Author

ghoz commented Jun 12, 2024

Unfortunately too many tests timeout on my way too underpowered system :-/ I'll have to get a proper dev setup at some point
From the CI logs, it seems to be complaining about line 165 which is the actual function definition ... not sure I understand..
function removeNullPropertiesFromObject(obj: KeyValue): void {

lib/util/utils.ts Outdated Show resolved Hide resolved
lib/util/settings.ts Outdated Show resolved Hide resolved
@Nerivec
Copy link
Collaborator

Nerivec commented Jun 12, 2024

Coverage likely isn't happy because nothing must be testing the function called without that second param -defaulting- (all occurrences in z2m code pass the param).
Something like this in utils.test.js should do:

    it('Removes null properties from object', () => {
        const obj1 = {
            ab: 0,
            cd: false,
            ef: null,
            gh: '',
            homeassistant: {
                xyz: 'mock',
                abcd: null,
            },
            nested: {
                homeassistant: {
                    abcd: true,
                    xyz: null,
                },
                abc: {},
                def: null,
            },
        };

        utils.removeNullPropertiesFromObject(obj1);
        expect(obj1).toStrictEqual({
            ab: 0,
            cd: false,
            gh: '',
            homeassistant: {
                xyz: 'mock',
            },
            nested: {
                homeassistant: {
                    abcd: true,
                },
                abc: {}
            },
        });

        const obj2 = {
            ab: 0,
            cd: false,
            ef: null,
            gh: '',
            homeassistant: {
                xyz: 'mock',
                abcd: null,
            },
            nested: {
                homeassistant: {
                    abcd: true,
                    xyz: null,
                },
                abc: {},
                def: null,
            },
        };
        utils.removeNullPropertiesFromObject(obj2, ['homeassistant']);
        expect(obj2).toStrictEqual({
            ab: 0,
            cd: false,
            gh: '',
            homeassistant: {
                xyz: 'mock',
                abcd: null,
            },
            nested: {
                homeassistant: {
                    abcd: true,
                    xyz: null,
                },
                abc: {}
            },
        });
    });

@ghoz ghoz force-pushed the fix-removed-ha-discovery-null branch from 341f0e3 to 8c606d5 Compare June 14, 2024 20:46
@ghoz
Copy link
Contributor Author

ghoz commented Jun 14, 2024

Implemented @Nerivec suggestions, thanks!

@Nerivec
Copy link
Collaborator

Nerivec commented Jun 14, 2024

Something went wrong with your latest commit.

@ghoz ghoz force-pushed the fix-removed-ha-discovery-null branch from 8c606d5 to d65a611 Compare June 14, 2024 21:13
@ghoz
Copy link
Contributor Author

ghoz commented Jun 14, 2024

Something went wrong with your latest commit.

I messed up trying to rebase , not sure what went wrong...
Thanks!

@Koenkk Koenkk merged commit d41cf43 into Koenkk:dev Jun 15, 2024
11 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Jun 15, 2024

Thanks both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants