Fix gameplay test being mistakenly erased and improve layer error messages (#9124)

newIDE/app/src/EditorFunctions/GameplayTestTools.js

@@ -41,6 +41,21 @@ const invalidScopeFailure = () =>
     "Invalid `scope`: pass { type: 'project' } or { type: 'extension', extension_name: '...' }."
   );
 
+/**
+ * The test code asked for by a `run_gameplay_test` call, or null to run the
+ * stored test as-is.
+ *
+ * A blank `source` means "run the stored test": models routinely send
+ * `source: ""` instead of omitting the optional argument, and taking that
+ * literally would overwrite the stored test with an empty body (and run a
+ * no-op that reports "it did nothing"). An empty test body is never
+ * something to store nor to run, so it can safely be read as absent.
+ */
+const parseSourceArgument = (sourceArgument: mixed): string | null =>
+  typeof sourceArgument === 'string' && sourceArgument.trim()
+    ? sourceArgument
+    : null;
+
 /**
  * The output sent to the AI for a gameplay test run: the full result of the
  * run, with console logs flattened to strings.
@@ -95,7 +110,7 @@ export const runGameplayTest: EditorFunction = {
     const testName = args.test_name || '';
     return {
       text:
-        args.source && args.persist !== false ? (
+        parseSourceArgument(args.source) !== null && args.persist !== false ? (
           <Trans>Save and run the gameplay test {testName}.</Trans>
         ) : (
           <Trans>Run the gameplay test {testName}.</Trans>
@@ -109,7 +124,7 @@ export const runGameplayTest: EditorFunction = {
     if (typeof testName !== 'string' || !testName) {
       return makeFailure('Missing or invalid `test_name` argument.');
     }
-    const source = typeof args.source === 'string' ? args.source : null;
+    const source = parseSourceArgument(args.source);
     const persist = args.persist !== false;
     const timeoutMs =
       typeof args.timeout_ms === 'number'
@@ -199,7 +214,7 @@ export const runGameplayTest: EditorFunction = {
   // neither does running an unsaved source with `persist: false` (temporary
   // probes and diagnostics).
   getModifiesProject: (args: Object) =>
-    typeof args.source === 'string' && args.persist !== false,
+    parseSourceArgument(args.source) !== null && args.persist !== false,
 };
 
 // Cap on the ordered test list returned after changes (mirrors the array

newIDE/app/src/EditorFunctions/GameplayTestTools.spec.js

@@ -0,0 +1,220 @@
+// @flow
+import { editorFunctions, type EditorFunctionGenericOutput } from './index';
+import { makeFakeLaunchFunctionOptionsWithProject } from './TestHelpers';
+
+// The real runner needs a preview launcher (and a browser): only the call it
+// receives matters here, so it is replaced by a fake recording it.
+const mockRunProjectGameplayTests = jest.fn<Array<any>, any>();
+jest.mock('../GameplayTests/GameplayTestRunner', () => ({
+  ...jest.requireActual('../GameplayTests/GameplayTestRunner'),
+  runProjectGameplayTests: (...args: Array<any>) =>
+    mockRunProjectGameplayTests(...args),
+}));
+
+const gd: libGDevelop = global.gd;
+
+const STORED_SOURCE = 'await harness.goToScene("Scene");';
+
+const makePassedResult = (testName: string) => ({
+  testName,
+  status: 'passed',
+  framesExecuted: 10,
+  durationMs: 100,
+  loadingMs: 10,
+  timeoutMs: 30000,
+  hiddenStallMs: 0,
+  gameTimeMs: 160,
+  assertions: [],
+  errors: [],
+  consoleLogs: [],
+  eventLog: [],
+  finalState: null,
+});
+
+describe('run_gameplay_test', () => {
+  let project: gdProject;
+
+  beforeEach(() => {
+    // $FlowFixMe[invalid-constructor]
+    project = new gd.ProjectHelper.createNewGDJSProject();
+    mockRunProjectGameplayTests.mockReset();
+    mockRunProjectGameplayTests.mockImplementation(({ tests }) =>
+      Promise.resolve([makePassedResult(tests[0].testName)])
+    );
+  });
+
+  afterEach(() => {
+    project.delete();
+  });
+
+  const addStoredTest = (testName: string) => {
+    const testsContainer = project.getTests();
+    const test = testsContainer.insertNewTest(
+      testName,
+      testsContainer.getTestsCount()
+    );
+    test.setSource(STORED_SOURCE);
+    return test;
+  };
+
+  const launch = (args: Object): Promise<EditorFunctionGenericOutput> =>
+    editorFunctions.run_gameplay_test.launchFunction({
+      ...makeFakeLaunchFunctionOptionsWithProject(project),
+      args,
+    });
+
+  const getRunTest = () =>
+    mockRunProjectGameplayTests.mock.calls[0][0].tests[0];
+
+  it('runs the stored test as-is when `source` is omitted', async () => {
+    addStoredTest('MyTest');
+
+    const result = await launch({
+      scope: { type: 'project' },
+      test_name: 'MyTest',
+    });
+
+    expect(result.success).toBe(true);
+    expect(getRunTest().source).toBe(undefined);
+    expect(
+      project
+        .getTests()
+        .getTest('MyTest')
+        .getSource()
+    ).toBe(STORED_SOURCE);
+    expect(result.meta).toBe(undefined);
+  });
+
+  it.each([['an empty string', ''], ['whitespace only', '   \n  ']])(
+    'runs the stored test as-is (and keeps its source) when `source` is %s',
+    async (_label, source) => {
+      addStoredTest('MyTest');
+
+      const result = await launch({
+        scope: { type: 'project' },
+        test_name: 'MyTest',
+        source,
+        description: '',
+        persist: true,
+      });
+
+      expect(result.success).toBe(true);
+      // The blank source must never reach the runner nor the project.
+      expect(getRunTest().source).toBe(undefined);
+      expect(
+        project
+          .getTests()
+          .getTest('MyTest')
+          .getSource()
+      ).toBe(STORED_SOURCE);
+      expect(result.meta).toBe(undefined);
+    }
+  );
+
+  it('does not create an empty test when `source` is blank and no test exists', async () => {
+    const result = await launch({
+      scope: { type: 'project' },
+      test_name: 'MissingTest',
+      source: '',
+      persist: true,
+    });
+
+    expect(result.success).toBe(false);
+    expect(result.message).toContain('No test named "MissingTest"');
+    expect(project.getTests().hasTestNamed('MissingTest')).toBe(false);
+    expect(mockRunProjectGameplayTests).not.toHaveBeenCalled();
+  });
+
+  it('still creates and runs a test when a real `source` is given', async () => {
+    const newSource = 'await harness.stepFrames(5);';
+
+    const result = await launch({
+      scope: { type: 'project' },
+      test_name: 'NewTest',
+      source: newSource,
+      description: 'Checks something',
+    });
+
+    expect(result.success).toBe(true);
+    expect(getRunTest().source).toBe(newSource);
+    expect(
+      project
+        .getTests()
+        .getTest('NewTest')
+        .getSource()
+    ).toBe(newSource);
+    expect(
+      project
+        .getTests()
+        .getTest('NewTest')
+        .getDescription()
+    ).toBe('Checks something');
+    expect(result.meta).toEqual({ didModifyProject: true });
+  });
+
+  it('still updates an existing test when a real `source` is given', async () => {
+    addStoredTest('MyTest');
+    const newSource = 'await harness.stepFrames(42);';
+
+    const result = await launch({
+      scope: { type: 'project' },
+      test_name: 'MyTest',
+      source: newSource,
+    });
+
+    expect(result.success).toBe(true);
+    expect(getRunTest().source).toBe(newSource);
+    expect(
+      project
+        .getTests()
+        .getTest('MyTest')
+        .getSource()
+    ).toBe(newSource);
+    expect(result.meta).toEqual({ didModifyProject: true });
+  });
+
+  it('runs an unsaved source without touching the stored test when `persist` is false', async () => {
+    addStoredTest('MyTest');
+    const probeSource = 'await harness.stepFrames(1);';
+
+    const result = await launch({
+      scope: { type: 'project' },
+      test_name: 'MyTest',
+      source: probeSource,
+      persist: false,
+    });
+
+    expect(result.success).toBe(true);
+    expect(getRunTest().source).toBe(probeSource);
+    expect(
+      project
+        .getTests()
+        .getTest('MyTest')
+        .getSource()
+    ).toBe(STORED_SOURCE);
+    expect(result.meta).toBe(undefined);
+  });
+
+  describe('getModifiesProject', () => {
+    it.each([
+      ['a blank source does not modify the project', { source: '' }, false],
+      [
+        'a whitespace source does not modify the project',
+        { source: '  ' },
+        false,
+      ],
+      ['no source does not modify the project', {}, false],
+      ['a real source modifies the project', { source: STORED_SOURCE }, true],
+      [
+        'a real source not persisted does not modify the project',
+        { source: STORED_SOURCE, persist: false },
+        false,
+      ],
+    ])('%s', (_label, args, expected) => {
+      expect(
+        editorFunctions.run_gameplay_test.getModifiesProject &&
+          editorFunctions.run_gameplay_test.getModifiesProject(args)
+      ).toBe(expected);
+    });
+  });
+});

newIDE/app/src/EditorFunctions/Put2dInstances.spec.js

@@ -70,6 +70,33 @@ describe('put_2d_instances (modifications of existing instances)', () => {
     return result;
   };
 
+  it('lists the existing layers when the layer does not exist', async () => {
+    testScene.insertNewLayer('UI', testScene.getLayersCount());
+
+    const result: EditorFunctionGenericOutput = await editorFunctions.put_2d_instances.launchFunction(
+      {
+        ...makeFakeLaunchFunctionOptionsWithProject(project),
+        args: {
+          scene_name: 'TestScene',
+          object_name: 'Player',
+          layer_name: 'HUD',
+          brush_kind: 'point',
+          brush_position: '10,20',
+        },
+      }
+    );
+
+    expect(result.success).toBe(false);
+    expect(result.message).toBe(
+      'Layer not found: HUD in scene "TestScene". ' +
+        'Existing layers: "" (the base layer), "UI". ' +
+        'Use one of them, or add the layer first with ' +
+        '`change_scene_properties_layers_effects_groups` (a `changed_layers` ' +
+        'entry creates the layer when its `layer_name` does not exist yet).'
+    );
+    expect(getInstances(testScene)).toHaveLength(0);
+  });
+
   it('moves an existing instance to a different layer with the none brush', async () => {
     testScene.insertNewLayer('UI', testScene.getLayersCount());
     await putInstances({

newIDE/app/src/EditorFunctions/ScopeInstancesAndLayers.spec.js

@@ -156,8 +156,14 @@ describe('scoped instances, layers and groups', () => {
       );
 
       expect(result.success).toBe(false);
+      // The layers of the associated scene are listed, so the next call can
+      // use an existing one instead of guessing another name.
       expect(result.message).toBe(
-        'Layer not found: NotALayer in external layout "LevelChunk".'
+        'Layer not found: NotALayer in external layout "LevelChunk". ' +
+          'Existing layers: "" (the base layer), "Background". ' +
+          'Use one of them, or add the layer first with ' +
+          '`change_scene_properties_layers_effects_groups` (a `changed_layers` ' +
+          'entry creates the layer when its `layer_name` does not exist yet).'
       );
       expect(
         project

newIDE/app/src/EditorFunctions/Utils.js

@@ -1,6 +1,6 @@
 // @flow
 import { type AssetShortHeader } from '../Utils/GDevelopServices/Asset';
-import { mapVector } from '../Utils/MapFor';
+import { mapFor, mapVector } from '../Utils/MapFor';
 import { SafeExtractor } from '../Utils/SafeExtractor';
 import { serializeToJSObject } from '../Utils/Serializer';
 import { type EditorFunctionGenericOutput } from './index';
@@ -312,6 +312,32 @@ export const makeGenericFailure = (
   message,
 });
 
+/**
+ * The failure for a layer that does not exist, naming the layers that do.
+ *
+ * Without the list, the only way out of this error is to guess another name,
+ * and a conventional one ("UI", "Background", "HUD") is guessed again and
+ * again on scenes that only have a base layer. Listing the real layers and
+ * how to add one makes the next call the right one.
+ */
+export const makeLayerNotFoundFailure = (
+  layerName: string,
+  scopeLabel: string,
+  layersContainer: gdLayersContainer
+): EditorFunctionGenericOutput => {
+  const existingLayerNames = mapFor(0, layersContainer.getLayersCount(), i =>
+    layersContainer.getLayerAt(i).getName()
+  ).map(name => (name === '' ? '"" (the base layer)' : `"${name}"`));
+
+  return makeGenericFailure(
+    `Layer not found: ${layerName} in ${scopeLabel}. ` +
+      `Existing layers: ${existingLayerNames.join(', ')}. ` +
+      'Use one of them, or add the layer first with ' +
+      '`change_scene_properties_layers_effects_groups` (a `changed_layers` ' +
+      'entry creates the layer when its `layer_name` does not exist yet).'
+  );
+};
+
 export const shouldHideProperty = (property: gdPropertyDescriptor): boolean => {
   return (
     property.isHidden() ||

newIDE/app/src/EditorFunctions/index.js

@@ -92,6 +92,7 @@ import {
   getObjectSizeInfoHints,
   getSimplifiedInstance,
   makeGenericFailure,
+  makeLayerNotFoundFailure,
   shouldHideProperty,
   type ObjectSizeInfo,
 } from './Utils';
@@ -4407,8 +4408,10 @@ const put2dInstances: EditorFunction = {
 
     // Check if layer exists (empty string is allowed for base layer)
     if (layerName !== '' && !layersContainer.hasLayerNamed(layerName)) {
-      return makeGenericFailure(
-        `Layer not found: ${layerName} in ${resolvedScope.label}.`
+      return makeLayerNotFoundFailure(
+        layerName,
+        resolvedScope.label,
+        layersContainer
       );
     }
 
@@ -5380,8 +5383,10 @@ const put3dInstances: EditorFunction = {
 
     // Check if layer exists (empty string is allowed for base layer)
     if (layerName !== '' && !layersContainer.hasLayerNamed(layerName)) {
-      return makeGenericFailure(
-        `Layer not found: ${layerName} in ${resolvedScope.label}.`
+      return makeLayerNotFoundFailure(
+        layerName,
+        resolvedScope.label,
+        layersContainer
       );
     }