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

Fill test explorer with mutations on startup #1

Closed
wants to merge 13 commits into from

Conversation

jaspervdveen
Copy link
Owner

@jaspervdveen jaspervdveen commented Mar 22, 2024

Some limitations:

Copy link
Collaborator

@danny12321 danny12321 left a comment

Choose a reason for hiding this comment

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

Ik denk dat je een goede eerste opzet hebt gemaakt. Denk dat we nog wel wat beters moeten verzinnen voor het ondersteunen van verschillende platformen, aangezien 1 class met daarin alle logica van het platform mij een grote class lijkt worden. Mocht je het niet eens zijn met mijn comments laat het vooral weten.


export class PlatformFactory {

public getPlatform(): Platform {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kan dit niet een static function zijn? En moeten we hier uberhaupt een class van maken?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Kan dit niet een static function zijn?

Ja, klopt. Ik heb het nu aangepast.

En moeten we hier uberhaupt een class van maken?

Ik verwacht dat deze factory later uitgebreid wordt met methodes om te bepalen welk Stryker-platform aangeroepen moet worden. Om die reden had ik hem al aangemaakt, maar een klasse is nu niet direct nodig. Ik kan hem eventueel weghalen als je wilt, maar het was alvast een opzet voor later.

import { ProgressLocation, window } from "vscode";
import { MutationTestResult } from "mutation-testing-report-schema";

export class StrykerJs implements Platform {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deze naam is nu wel heel generiek. Als we in de class alle functionaliteit van StrykerJS gaan zetten lijkt het mij een groot bestand te worden.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ik wil deze klasse enkel gebruiken voor de verschillende aanroepen van de executable, en vervolgens de resultaten daaruit afhandelen in andere klassen. Ik denk dat hij daarom niet groot zal worden. Ook kan ik nog functionaliteit afsplitsen naar de base class Platform, want ik verwacht dat het gros vergelijkbaar zal zijn tussen de Stryker-platformen.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ik heb nu ook het e.e.a. verplaatst naar de base abstract class

src/platforms/strykerjs.ts Outdated Show resolved Hide resolved
src/platforms/strykerjs.ts Outdated Show resolved Hide resolved
}, async () => {
try {
const command = `${this.executable} run --instrumentRunOnly --checkers ""` +
` --buildCommand "" --plugins "" --testRunner "" --logLevel off --reporters json`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ik wil voorstellen om dit net zo op te zetten als het spawen van een childprocess. Een string is lastiger te manipuleren dan een array (van argumenten).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes dat is beter! Ik heb het aangepast.
Ik gebruik nu ook spawn i.p.v. exec omdat dit een stuk sneller blijkt te zijn.

src/extension.ts Outdated Show resolved Hide resolved
@jaspervdveen
Copy link
Owner Author

Closed vwg andere approach met mutation server, PR is niet meer relevant

@jaspervdveen jaspervdveen deleted the feat/pending-mutations-test-explorer branch April 29, 2024 16:12
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