MagicMirror icon indicating copy to clipboard operation
MagicMirror copied to clipboard

Update compliments with support for cron type date/time for selections, addition to just date.

Open sdetweil opened this issue 1 year ago • 14 comments

  • What does the pull request accomplish? Use a list if needed.

this change allows uses to configure date/time events for compliments.. also linked site that will build the cron entry..

and example was Happy hour in a pub, on fri/sat between 5 and 7 pm.

or just after midnight on Halloween (Boooooo!)

I also added testcases for #3478

(and added support for this in MMM-Config), with a custom, drop down selection list of the types.. )

| if this is approved I will update the module doc

sdetweil avatar Jun 24 '24 23:06 sdetweil

the same failing 2 tests run correctly in the electron test suite.. what is the problem here?

copied from the electron test suite and added the get.document() call before the expect. but only these two fail. like the date isn't set

sdetweil avatar Jun 25 '24 03:06 sdetweil

ok, the problem here is the startApplication() helper in e2e does not accept dates for the test.. the electron version does accept date

so we cannot run date based testcases in e2e .. the electron tests DO pass

sdetweil avatar Jun 25 '24 15:06 sdetweil

the failing testcase depends on the weather...

	it("should render a compliment based on the current weather", async () => {
    > 41 | 			await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
         | 			      ^
      42 | 		});

this is a bad testcase.. but not related to my changes.

sdetweil avatar Jun 25 '24 15:06 sdetweil

the weather testcase depends on a weather provider responding to the special value in the config

		{
			module: "weather",
			position: "bottom_bar",
			config: {
				location: "Munich",
				mockData: '"#####WEATHERDATA#####"'
			}
		}

but the default provider does not send any notification..

sdetweil avatar Jun 25 '24 16:06 sdetweil

the failure is the weather test, untouched by me

  Received promise rejected instead of resolved
    Rejected to value: [Error: expect(received).toBe(expected) // Object.is equality·
    Expected: "snow"
    Received: ""]

      39 |
      40 | 		it("should render a compliment based on the current weather", async () => {
    > 41 | 			await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
         | 			      ^
      42 | 		});
      43 | 	});

sdetweil avatar Jun 26 '24 18:06 sdetweil

untouched, but you changed compliment sources

khassel avatar Jun 26 '24 19:06 khassel

yes, but that testcase is in weather.. not in compliments and that part of compliments wasnt changed. it depends on the notification

	describe("Compliments Integration", () => {
		beforeAll(async () => {
			await weatherFunc.startApp("tests/configs/modules/weather/currentweather_compliments.js", {});
		});

		it("should render a compliment based on the current weather", async () => {
			await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
		});
	});

i tested and the compliments notification_received is not triggered..

in compliments.js

		// Add compliments based on weather
		if (this.currentWeatherType in this.config.compliments) {
			Array.prototype.push.apply(compliments, this.config.compliments[this.currentWeatherType]);
			// if the predefine list doesn't include it (yet)             // i added these two lines
			if (!this.pre_defined_types.includes(this.currentWeatherType)) {
				// add it
				this.pre_defined_types.push(this.currentWeatherType);
			}
		}

		// Add compliments for anytime
		Array.prototype.push.apply(compliments, this.config.compliments.anytime);

the config for the weather_compliments testcase

let config = {
	modules: [
		{
			module: "compliments",
			position: "top_bar",
			config: {
				compliments: {
					snow: ["snow"]
				},
				updateInterval: 3000
			}
		},
		{
			module: "weather",
			position: "bottom_bar",
			config: {
				location: "Munich",
				mockData: '"#####WEATHERDATA#####"'
			}
		}
	]
};

doesn't have date based, and doesn't have normal ,, so depends on the notification, which doesn't arrive...

	// Override notification handler.
	notificationReceived (notification, payload, sender) {
		if (notification === "CURRENTWEATHER_TYPE") {
			this.currentWeatherType = payload.type;
		}
	}

I forced this.currentWeatherType to 'snow' with a compliment under "snow" and it appeared.. so the compliments module is not broken

sdetweil avatar Jun 26 '24 19:06 sdetweil

with old compliments.js:

node@cb3498f2205a:/opt/magic_mirror$ npx jest tests/e2e/modules/weather_current_spec.j --force-exit
 PASS   e2e  tests/e2e/modules/weather_current_spec.js (21.22 s)
  Weather module
    Current weather
      Default configuration
        ✓ should render wind speed and wind direction (814 ms)
        ✓ should render temperature with icon (201 ms)
        ✓ should render feels like temperature (200 ms)
        ✓ should render humidity next to feels-like (201 ms)
    Compliments Integration
      ✓ should render a compliment based on the current weather (5315 ms)
    Configuration Options
      ✓ should render windUnits in beaufort (304 ms)
      ✓ should render windDirection with an arrow (206 ms)
      ✓ should render humidity next to wind (204 ms)
      ✓ should render degreeLabel for temp (203 ms)
      ✓ should render degreeLabel for feels like (202 ms)
    Current weather with imperial units
      ✓ should render wind in imperial units (306 ms)
      ✓ should render temperatures in fahrenheit (201 ms)
      ✓ should render 'feels like' in fahrenheit (203 ms)

Test Suites: 1 passed, 1 total
Tests:       13 passed, 13 total
Snapshots:   0 total
Time:        21.85 s
Ran all test suites matching /tests\/e2e\/modules\/weather_current_spec.j/i.

with compliments.js of this PR:

node@cb3498f2205a:/opt/magic_mirror$ npx jest tests/e2e/modules/weather_current_spec.j --force-exit
 FAIL   e2e  tests/e2e/modules/weather_current_spec.js (6.252 s)
  Weather module
    Current weather
      Default configuration
        ✓ should render wind speed and wind direction (305 ms)
        ✓ should render temperature with icon (201 ms)
        ✓ should render feels like temperature (202 ms)
        ✓ should render humidity next to feels-like (202 ms)
    Compliments Integration
      ✕ should render a compliment based on the current weather (212 ms)
    Configuration Options
      ✓ should render windUnits in beaufort (308 ms)
      ✓ should render windDirection with an arrow (206 ms)
      ✓ should render humidity next to wind (200 ms)
      ✓ should render degreeLabel for temp (203 ms)
      ✓ should render degreeLabel for feels like (203 ms)
    Current weather with imperial units
      ✓ should render wind in imperial units (300 ms)
      ✓ should render temperatures in fahrenheit (202 ms)
      ✓ should render 'feels like' in fahrenheit (200 ms)

  ● Weather module › Compliments Integration › should render a compliment based on the current weather

    expect(received).resolves.toBe()

    Received promise rejected instead of resolved
    Rejected to value: [Error: expect(received).toBe(expected) // Object.is equality·
    Expected: "snow"
    Received: ""]

      39 |
      40 |              it("should render a compliment based on the current weather", async () => {
    > 41 |                      await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
         |                            ^
      42 |              });
      43 |      });
      44 |

      at expect (node_modules/expect/build/index.js:113:15)
      at Object.expect (tests/e2e/modules/weather_current_spec.js:41:10)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 12 passed, 13 total
Snapshots:   0 total
Time:        6.297 s
Ran all test suites matching /tests\/e2e\/modules\/weather_current_spec.j/i.

khassel avatar Jun 26 '24 19:06 khassel

looking.. how do you debug the code here?

sdetweil avatar Jun 26 '24 20:06 sdetweil

looking.. how do you debug the code here?

within the test? This is difficult, AFAIR only option is console.dir()

khassel avatar Jun 26 '24 20:06 khassel

and how to debug.. i had to create the config.js used by the test after copying the mock data, so I could test it in a real MM run..

sdetweil avatar Jun 27 '24 13:06 sdetweil

updateInterval from testcases for #3471, I didn't check that ,, fixed header.. oops I started with a prior version,, didn't catch that.. fixed

sdetweil avatar Jun 27 '24 20:06 sdetweil

done

sdetweil avatar Jun 27 '24 20:06 sdetweil

because I had to fix the prior testcases, it made me realize there is a cron anytime as well.. so I added that to e2e configs and tests

sdetweil avatar Jun 27 '24 21:06 sdetweil

@rejas please review. I'd love your feedback

sdetweil avatar Jul 09 '24 17:07 sdetweil