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

fix #3267, CORRECTLY, add testcase, add testcase for #3279 #3681

Merged
merged 13 commits into from
Jan 5, 2025

Conversation

sdetweil
Copy link
Collaborator

@sdetweil sdetweil commented Jan 4, 2025

fixes #3267 AGAIN, correctly, add testcase
add testcase for #3679 , broadcast clipping incorrectly

I added a test module (tests/testNotification) to catch the notification and check the count. (one way to configure)
i put this module in the tests folder, and added /tests to the server paths.
(can't have a module in a nested folder, like tests/modules/xxx)

but I have a problem. i can run the test config (MM_CONFIG_FILE), and the two modules work correctly,
but in the spec runner, the calendar module times out on the broadcast test.. there is only local ics file access, no outside hosts

I forced my system date to 1/1/24 (same as runner) and again the manual testcase works fine

I added two test config.js,(configs/calendar) one works great (symbols) , one fails (broadcast test)
I added additional delay in the calendarspec runner to try to debug the module, but it still not long enough.. no messages of trouble when I get into the browser.. BUT, this may be because of the log being turned off... (just thought of this)

I created a special ICS (in mocks) that has 12 events, 1 for each month.. (so I can check clipping and broadcast) the US holidays one is sensitive to the current date, and I couldn't get it to work on 1/1/2024..

also, in general, is there a mechanism to run test:just_one_runner? waiting thru the electron test to get to one testcase.. ugh..

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

Ok, the problem is here in app.js

	function loadModule (module) {
		const elements = module.split("/");
		const moduleName = elements[elements.length - 1];
		const env = getEnvVarsAsObj();
		let moduleFolder = path.resolve(`${__dirname}/../${env.modulesDir}`, module);

		if (defaultModules.includes(moduleName)) {
			const defaultModuleFolder = path.resolve(`${__dirname}/../modules/default/`, module);
			if (process.env.JEST_WORKER_ID === undefined) {
				moduleFolder = defaultModuleFolder;
			} else {
				// running in Jest, allow defaultModules placed under moduleDir for testing
				if (env.modulesDir === "modules") {
					moduleFolder = defaultModuleFolder;
				}
			}
		}

so I am using two modules
calendars.js (default module)
tests/testNotification foreignModulesDir: "tests"

so moduleFolder never gets set for defaultModuleFolder.
I don't WANT modules to the the location for this test only module

if I change the test like this

if (env.modulesDir === "modules" || env.modulesDir === "tests") {

then it works with the foreignModulesDir:"tests"

is this acceptable?

@khassel
Copy link
Collaborator

khassel commented Jan 5, 2025

I'm not happy with these changes.

You could move the special testNotification stuff beside the electron test and then copy it into the needed folder before doing the test, similar to this in neewsfeed test.

This would avoid the changes in app.js and server.js.

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

I think we are talking about different problems..

the testcase needs to receive a notification, sent by calendar. and the testcase needs to examine the data sent,
but it is not visible to the testcase

also, the same module cannot receive notifications it sends... so I can't put code IN calendar to access that data

so I need s NEW module that can receive the notification, and present some web content that the testcase runner can examine

ok, now I have this new module, where to put it ...

I originally put it in defaults, but its not a general purpose module, its ONLY for use during test.

so, put it in tests, but there is no server path to tests, so it can't be loaded.
fix that (server.js) but then the defaults path is broken (app.js)

are you proposing I copy the module from tests folder to modules folder? ok, but then this testcase cannot be run on its own without the user KNOWing they have to copy the folder first..

@khassel
Copy link
Collaborator

khassel commented Jan 5, 2025

but then this testcase cannot be run on its own without the user KNOWing they have to copy the folder first..

you do the copy inside the test case with e.g. fs.cpSync as already done in newsfeed test, see link in my comment above

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

yes, i have that working

can you explain what your concern is w having the addition test assets IN the tests folder. and exposing that? (we do that for all the other test assets, you added the config and mocks to be accessible outside test)

this is different than the newsfeed test , where you NEED the module to be in a different location to test the foreignModuleDir property

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

you do the copy inside the test case with e.g. fs.cpSync as already done in newsfeed test, see link in my comment above

yes,
we exposed config/mocks
so you can do
export MM_CONFIG_FILE=the_testcase_config
npm start and it will work
that will NOT work for this test as the copy is done (under the covers) in the runner

@khassel
Copy link
Collaborator

khassel commented Jan 5, 2025

other possibility (instead of the copy) would to move the testNotification into an already published tests folder.

this makes no sense:

let directories = ["/config", "/css", "/fonts", "/modules", "/vendor", "/translations", "/tests/configs", "/tests/mocks", "/tests"];

if we really need to publish the whole tests folder the 2 subfolders should be removed.

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

yes, testNotification module folder is ALREADY in tests in the PR as submitted

I admit that I screwed up.. and should have removed the explicit sub folder shares.

@khassel
Copy link
Collaborator

khassel commented Jan 5, 2025

I would move testNotification under tests/mocks

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

Ok, moved, and we are back to this hack in app.js

		let moduleFolder = path.resolve(`${__dirname}/../${env.modulesDir}`, module);  ///assume module is in 'modules' folder

		if (defaultModules.includes(moduleName)) {  // if its a default module
			const defaultModuleFolder = path.resolve(`${__dirname}/../modules/default/`, module); // calc new path
			if (process.env.JEST_WORKER_ID === undefined) {  // if not in test mode
				moduleFolder = defaultModuleFolder;   // use the defaults folder
			} else {
				// running in Jest, allow defaultModules placed under moduleDir for testing
				if (env.modulesDir === "modules" || env.modulesDir === "tests/mocks") { //<--

					moduleFolder = defaultModuleFolder;
				}
			}
		}

//<-- have to add, else non-default modules are ONLY allowed in wherever 'modules' folder is set, so we would be back to copy
the config works ok outside test without this
this testcase is failing on the default calendar module not being found (as its not in 'modules' folder, set as default)

but this exposes using a default module for 3rd party module location test..

this is set in the testcase config, as module search is not recursive..

foreignModuleDir:"tests/mocks"

js/server.js Outdated Show resolved Hide resolved
tests/electron/modules/calendar_spec.js Outdated Show resolved Hide resolved
remove test comment
@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

after sync and pull (I changed a file directly and it had only spaces..)

anyhow

tests don't run

platform

Linux sams 5.15.0-127-generic #137-Ubuntu SMP Fri Nov 8 15:21:01 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
(base) sam@sams:~/MagicMirror.old$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 22.04.5 LTS
Release:	22.04
Codename:	jammy

output

    electron.launch: Process failed to launch!

      16 | 	electronParams.unshift("js/electron.js", "--enable-features=UseOzonePlatform", "--ozone-platform=wayland");
      17 |
    > 18 | 	global.electronApp = await electron.launch({ args: electronParams });
         | 	                     ^
      19 |
      20 | 	await global.electronApp.firstWindow();
      21 |

suite summary

Test Suites: 4 failed, 4 total
Tests:       46 failed, 46 total
Snapshots:   0 total
Time:        37.717 s

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2025

I edited on the web directly so I wouldn't drag in the new runner problem..

@khassel khassel merged commit 5e337f8 into MagicMirrorOrg:develop Jan 5, 2025
11 checks passed
@khassel khassel mentioned this pull request Jan 5, 2025
rejas pushed a commit that referenced this pull request Jan 8, 2025
- fix setup to run with xserver and labwc
- remove electronParams from calendar_spec.js (forgotten in
#3680)

fixes [running tests locally without labwc
installed](#3681 (comment))

follow up to #3680
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.

2 participants