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.

Sign up for GitHub

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

Jump to bottom

feature(WorkspaceValidationTests): add possible to create workspace-wide tests #4949

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

feature(WorkspaceValidationTests): add possible to create workspace-wide tests #4949

wants to merge 12 commits into from

Conversation

DarkWeird
Copy link
Contributor

@DarkWeird DarkWeird commented Nov 12, 2021 •
edited
Loading

Contains

Proof-of-concept Workspace-wide tests(a.k.a mega test)

There should be tests with validate assets/assets/loading/mechanics loading provided by engine.
Possible Future - modules can provide tests, which will applicable to dependents (like API usage validation)

Currently using MTE as module. not works without it.
Currently use JS as target module to test. just for demonstration purposes.

How to test

  1. init workspace with JS and MTE (temporary)
  2. gradlew :facades:WorkspaceValidation:test

Outstanding before merging

  • MTE as Subsystem?
    It should be fine: will grants more possibles, remove optional module
  • provide mechanism for using modules as test's parameter for loading (like runs tryToLoadBehavior test across all modules)
  • remove specific module-related test.

@github-actions github-actions bot added the Type: Improvement Request for or addition/enhancement of a feature label Nov 12, 2021
@DarkWeird DarkWeird added Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Size: M Medium-sized effort likely requiring some research and work in multiple areas Status: Needs Discussion Requires help discussing a reported issue or provided PR labels Nov 12, 2021
casals
casals previously approved these changes Nov 12, 2021
View reviewed changes
@casals casals dismissed their stale review November 12, 2021 15:15

dark found something weird

@casals casals self-requested a review November 12, 2021 16:43
@casals casals marked this pull request as draft November 12, 2021 16:43
@keturn
Copy link
Member

keturn commented Nov 12, 2021

Hmm, interesting, I need to think about it a bit more, but a few initial thoughts:

  1. On moving MTE: Yes, see Package as a library instead of a module Terasology/ModuleTestingEnvironment#39
  2. Why is this categorized under facades/?

@DarkWeird
Copy link
Contributor Author

Does MTE want to be a Subsystem

Maybe i named it wrong.
Not EngineSubsystem, but a some subsystem which provides additional functionality for modules.(MTE exactly provides test functions for module's tests)
It cannot be library - too connected it with TS's engine.

Why is this categorized under facades/

I think that right semantically.
Facade is something that runs engine and modules. (PC, Server, AWT, Ed etc)
This test-related facade, which can runs test over whole module space.

@DarkWeird DarkWeird marked this pull request as ready for review November 15, 2021 15:10
keturn
keturn reviewed Nov 15, 2021
View reviewed changes
Copy link
Member

@keturn keturn left a comment

Choose a reason for hiding this comment

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

I have not yet reviewed the extraordinary stream pipeline of voluminous indentation. 🙈

...dation/src/test/java/org/terasology/workspace/validation/WorkspaceModulesResolvingTests.java
Comment on lines +34 to +35
Set<Name> moduleIds = temporary.getRegistry()
.getModuleIds();
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I have been trying to deprecate ModuleManager#getRegistry, because for the most part, other things being able to mess with the registry that ModuleManager is responsible for managing is bad for business.

Here you are using it to get all the modules in the current workspace. We could add a method to return this sort of read-only collection of module IDs, that would be pretty safe.

I'm guessing the other use case we have for wanting to see all the workspace's modules is the Advanced Game Setup screen where you pick which modules to activate. That uses a lot more than just the ID... I haven't looked at it to find out what interface it really wants.

...dation/src/test/java/org/terasology/workspace/validation/WorkspaceModulesResolvingTests.java
Comment on lines +52 to +54
void resolveAndLoadPairModules(ModuleManager moduleManager, Name moduleName1, Name moduleName2) {
moduleManager.resolveAndLoadEnvironment(moduleName1, moduleName2);
Assertions.assertNotNull(moduleManager.getEnvironment());
Copy link
Member

Choose a reason for hiding this comment

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

Have you confirmed this can actually fail? I'm wondering because of Terasology/ModuleTestingEnvironment#69

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, i cannot.
I tried to break one dependency.. but then gradle fail to resolve dependencies...
I don't know how to break dependency without breaking gradle resolution...

...aceValidation/src/test/java/org/terasology/workspace/validation/WorkspaceBehaviorsTests.java Outdated

}

public static class EnginesAccessor extends Engines {
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here? Why do we have a subclass that doesn't actually have any custom behavior? 😰

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is special temporary magic for accessing protected methods. ;)
Engines is nice enough, but i cannot access to EngineCleaner

@DarkWeird
Copy link
Contributor Author

I have not yet reviewed the extraordinary stream pipeline of voluminous indentation.

This stream provides:
0. Module selection, then create tests per module

  1. Setup as test per module ( a bit hacky. Setup provides asset manager via AtomicReference)
  2. Asset list by type(2 types) per module.( A bit hacky, asset manager accessable only after (1) Then creates test per asset.
  3. Append teardown per module

@DarkWeird
Copy link
Contributor Author

@keturn

I have not yet reviewed the extraordinary stream pipeline of voluminous indentation. see_no_evil

Refactored :)
Code can be reused for any other assets

@skaldarnar
Copy link
Member

Some thoughts and notes on this as I was trying it out to help with the Behaviors/Pathfinding work:

  • Crash Reporter opens up on failed tests
  • runs a lot of tests (not sure what they are all about) which definitely need better names and descriptions

    23498 tests completed, 20 failed

  • the 20 failed tests did not help me to figure out what exactly is wrong ... 🤔

@DarkWeird
Copy link
Contributor Author

Crash Reporter opens up on failed tests

Mte-related known issue...

runs a lot of tests (not sure what they are all about) which definitely need better names and descriptions
23498 tests completed, 20 failed

If you runs it from idea you saw it better. It have structure..

I have Synthetic tests for setup mte and teardown mte there. Mostly fails related with invalid assets/misconfiguration classes will in setup part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keturn keturn keturn left review comments

@skaldarnar skaldarnar Awaiting requested review from skaldarnar

@jdrueckert jdrueckert Awaiting requested review from jdrueckert

@casals casals Awaiting requested review from casals

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Size: M Medium-sized effort likely requiring some research and work in multiple areas Status: Needs Discussion Requires help discussing a reported issue or provided PR Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DarkWeird @keturn @skaldarnar @casals