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

Update compliments module #3471

Merged
merged 10 commits into from
Jun 24, 2024

Conversation

WallysWellies
Copy link
Contributor

Fixes #3465

Add config option specialDayUnique that defaults to false and causes special day compliments to be added to the existing compliments array. Setting this option to true will only show the compliments that have been configured for that day.

Add config option "specialDaysUnique". If set to true, only compliments configured for special days will be shown on those days. Default setting is false.
Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

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

looks good. sorry for moving the goalpost, but might you also be able to add a test for this to the mm tests?

CHANGELOG.md Outdated Show resolved Hide resolved
WallysWellies and others added 2 commits June 18, 2024 09:01
Changes suggested by rejas after review

Co-authored-by: Veeck <github@veeck.de>
@WallysWellies
Copy link
Contributor Author

I believe I have done what you ask @rejas. Obviously these tests rely on using the new compliments.js file so I assume tests are done against the develop branch?

@WallysWellies WallysWellies requested a review from rejas June 18, 2024 14:38
@sdetweil
Copy link
Collaborator

where are the actual tests for this PR? I don't see any update to the e2e or electron test scripts.

@sdetweil
Copy link
Collaborator

i added my tests to the electron and e2e tests... but npm run test:??? unit/e2e/electron all show your test didn't run

 FAIL   unit  tests/unit/modules/default/compliments/compliments_cron_entry.js
  ● Test suite failed to run

    Your test suite must contain at least one test.

but this is in both e2e and electron test suites

	describe("Feature cron entry in compliments module", () => {
		beforeAll(async () => {
			await helpers.startApplication("tests/configs/modules/compliments/compliments_cron_entry.js", "06 May 2022 17:03:00 GMT");
			await helpers.getDocument();
		});

		it("shows 'just pub time' compliment on friday, between 16:00 and 19:10, between 00-10 minutes after the hour, only on fridays ", async () => {
				//await helpers.startApplication("tests/configs/modules/compliments/compliments_cron_entry.js", "06 May 2022 17:03:00 GMT");
				await expect(doTest(["just pub time"])).resolves.toBe(true);
		});
	});

@khassel
Copy link
Collaborator

khassel commented Jun 22, 2024

where are the actual tests for this PR? I don't see any update to the e2e or electron test scripts.

there are no tests in this PR only new config's

@WallysWellies
Copy link
Contributor Author

I'm sorry but I've never used GitHub before and I can't find any documentation to explain how to test configs in the way you seem to expect.

I looked in the tests folder and found example configs so I made mine in the same format and I tested them on my local install. All my commits are based on changes I have made on my live instance because I don't know any other way to do it and I wrote them into the appropriate file using the GitHub web interface (hence the repeated issues with spaces rather than tabs!).

If there is guidance on how to contribute to the project with technical steps please point me to it.

@khassel
Copy link
Collaborator

khassel commented Jun 23, 2024

I don't expect writing tests if you are not experienced with this ...

If there is guidance on how to contribute to the project with technical steps please point me to it.

... and we have no guide for writing tests at the moment.

So @rejas can we proceed without tests to have the feature in the next release or other ideas?

@sdetweil
Copy link
Collaborator

sdetweil commented Jun 23, 2024

I have a testcase for compliments, I can add his..

BUT.. I have a failure.. says 'expects(" ")

	describe("Feature cron entry in compliments module", () => {
		describe("Set date and empty compliments for anytime, morning, evening and afternoon", () => {
			it("shows 'just pub time' compliment on friday, between 16:00 and 19:10, between 00-10 minutes after the hour, only on fridays", async () => {
					await helpers.startApplication("tests/configs/modules/compliments/compliments_cron_entry.js"); //, "06 May 2022 17:03:00 GMT"
					await helpers.getDocument();
					await expect(doTest(["just pub time"])).resolves.toBe(true);
			});
		});
	});

from the e2e test

    remoteFile option
      ✓ should show compliments from a remote file (409 ms)
    Feature cron entry in compliments module
      Set date and empty compliments for anytime, morning, evening and afternoon
        ✕ shows 'just pub time' compliment on friday, between 16:00 and 19:10, between 00-10 minutes after the hour, only on fridays (724 ms)

  ● Compliments module › Feature cron entry in compliments module › Set date and empty compliments for anytime, morning, evening and afternoon › shows 'just pub time' compliment on friday, between 16:00 and 19:10, between 00-10 minutes after the hour, only on fridays

    expect(received).resolves.toBe()

    Received promise rejected instead of resolved
    Rejected to value: [Error: expect(received).toContain(expected) // indexOf·
    Expected value: " "   <---------- this is incorrect
    Received array: ["just pub time"]]  <---- this matches test result 

      61 | 					await helpers.startApplication("tests/configs/modules/compliments/compliments_cron_entry.js"); //, "06 May 2022 17:03:00 GMT"
      62 | 					await helpers.getDocument();
    > 63 | 					await expect(doTest(["just pub time"])).resolves.toBe(true);

why?
notice its right after the remote file option which works with the same structure (all the tests have same structure)

@WallysWellies
Copy link
Contributor Author

I fear I am becoming unqualified to add to this discussion but I have double-checked that my local files match those in the commit and I have retested locally and MM is working as I had expected:

  • Using compliments_specialDayUnique_false.js in place of my config.js file causes the special day message to be added to the existing array so I see it randomly cycling through all 4 messages - 3 from the "anytime" array and one from the "....-..-.." array.
  • Using compliments_specialDayUnique_true.js causes the special day message to replace the existing array so all I get is the one message from the "....-..-.." array.

I am also using the altered compliments.js file from the commit.

Sorry if this is becoming burdensome.

@sdetweil
Copy link
Collaborator

sdetweil commented Jun 23, 2024

@WallysWellies
I am learning too... the code runs, but the tests fail

npm run test:unit

because the files out there are supposed to be the test script, not the config for the test
the configs are supposed to be in tests/configs/modules/compliments folder

and there are three kinds of testcases

unit,,, - for when a module has sub-parts that return data to higher parts.. weather and calendar are like that
electron - that starts electron and grabs the module content and compares
and
e2e whch runs everything

i just discovered this yesterday, cause I haven't built any testcases before.. made a lot of changes, but all before testing

so you would move your two config.js files from the unit tree to the configs tree
and then edit the compliments_specs.js test case in the electron/modules/ folder
1 test using each config file

@sdetweil
Copy link
Collaborator

SO.. I want to do the test as a false result, BUT

this in the test process does NOT return false, it rejects

expect(complimentsArray).toContain(await elem.textContent());

and the test was

					await expect(doTest(["Special day message"])).resolves.toBe(false);

get

 ● Compliments module › Feature new parm for date events in compliments module › Set date and compliments for anytime, morning, evening and afternoon and one value for date › may show only date event on friday with new parm 

    expect(received).resolves.toBe()

    Received promise rejected instead of resolved
    Rejected to value: [Error: expect(received).toContain(expected) // indexOf·
    Expected value: "Typical message 1"
    Received array: ["Special day message"]]

      77 | 			it("may show only date event on friday with new parm ", async () => {
      78 | 					await helpers.startApplication("tests/configs/modules/compliments/compliments_specialDayUnique_false.js", "06 May 2022 14:03:00 GMT");
    > 79 | 					await expect(doTest(["Special day message"])).resolves.toBe(false);

i don't KNOW what the answer WILL be as its random.. I know it SHOULD NOT be 'Special day message'..
and the message is upside down on expect.. it reports as 'expected' what is in the thing being tested,
not on what it is tested with (in the parm TO the test (the array))..

how do I fix this?.. I don't know the test language at all.
all the prior tests are true results tests

@rejas
Copy link
Collaborator

rejas commented Jun 24, 2024

Will thinking about this, I dont think a test case is doable, since we present the compliments at random. So we cannot really check if the special-day message is shown exclusively or not... So maybe we skip tests for this for now ?

@khassel
Copy link
Collaborator

khassel commented Jun 24, 2024

I will present a test for this later or tomorrow

@khassel khassel requested review from rejas and removed request for rejas June 24, 2024 20:36
};

/*************** DO NOT EDIT THE LINE BELOW ***************/
if (typeof module !== "undefined") { module.exports = config; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

now youre just missing some actual test cases in the tests/e2e/modules/compliments_spec.js file, which load these configs and check the output of the MagicMirror

@rejas
Copy link
Collaborator

rejas commented Jun 24, 2024

Thanks @WallysWellies (and @khassel of course) for your work and your patience!

@rejas rejas merged commit aefb3a0 into MagicMirrorOrg:develop Jun 24, 2024
6 checks passed
@WallysWellies
Copy link
Contributor Author

I appreciate all your assistance and patience with me! It's been an experience dipping my toes into the world of Git and seeing how it's done. I've enjoyed making my small contribution to the project.

@WallysWellies WallysWellies deleted the update-compliments-module branch June 24, 2024 21:25
@sdetweil
Copy link
Collaborator

sdetweil commented Jun 24, 2024 via email

@sdetweil
Copy link
Collaborator

actually the negative could have ALL the choices to test against as positive. so any random result will work

@sdetweil
Copy link
Collaborator

actually that works

	describe("Feature new parm for date events in compliments module", () => {
		describe("Set date and compliments for anytime, morning, evening and afternoon and one value for date", () => {
			it("may show only date event on friday with new parm ", async () => {
					await helpers.startApplication("tests/configs/modules/compliments/compliments_specialDayUnique_false.js", "06 May 2022 14:03:00 GMT");
					await expect(doTest(["Special day message","Typical message 1","Typical message 2","Typical message 3"])).resolves.toBe(true);
			});
		});
	});

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.

4 participants