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

OfficeMockObject cannot mock NamedItemCollection #729

Open
3 tasks done
HarryPi opened this issue Jan 5, 2023 · 9 comments
Open
3 tasks done

OfficeMockObject cannot mock NamedItemCollection #729

HarryPi opened this issue Jan 5, 2023 · 9 comments

Comments

@HarryPi
Copy link

HarryPi commented Jan 5, 2023

Prerequisites

Please answer the following questions before submitting an issue.
YOU MAY DELETE THE PREREQUISITES SECTION.

  • I am running the latest version of Node and the tools
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

Expected behavior

Please describe the behavior you were expecting

I am trying to mock Named ranges array of excel so i can run some tests on adding and removing custom ranges, but it appears that the mock object is not equipped to handle arrays?

Using Jest and in Angular.

As per the documentation i am mocking the Host object and assigning it to the global.Excel

 it('should delete the first item', async () => {

    const workbookWithNamedRanges = {
      context: {
        workbook: {
          names: {
            items: [
              {
                name: 'Test 1',
                type: 'Error'
              },
              {
                name: 'Test 2',
                type: 'Error'
              },
            ]
          }
        }
      },
      run: async function (callback: any) {
        await callback(this.context);
      }
    };


    const excelMock = new OfficeMockObject(workbookWithNamedRanges) as any;

    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore
    global.Excel = excelMock;

    await service.service.removeFirstItem();

    expect(excelMock.context.workbook.names.items.length).toBe(1);
  });
});

Then in my service

async removeFirstItem(): Promise<void> {

    
    await Excel.run<void>(async context => {
      workbook = context.workbook;

      const namedRanges: Excel.NamedItemCollection = workbook.names.load() // <- Error here at .load;
      await context.sync();

      namedRanges.items.at(0)?.delete();

      await context.sync();


    });
}

The line mentioned above produces the following error

  Cannot convert undefined or null to object
TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)

Changing the code slightly to instead use context.load(workbook.names, 'items') produces a different error

Error: Property _properties needs to be present in object model before load is called.

    at C:\Users\User\Addins\addins\node_modules\office-addin-mock\src\officeMockObject.ts:168:15
    at Array.forEach (<anonymous>)
    at OfficeMockObject.parseObjectPropertyIntoArray (C:\Users\User\Addins\addins\node_modules\office-addin-mock\src\officeMockObject.ts:148:29)
    at OfficeMockObject.load (C:\Users\User\Addins\addins\node_modules\office-addin-mock\src\officeMockObject.ts:41:25)

Also trying workbook.names.load('items') works but items is undefined even though it was clearly defined in the mock object.

All of the ways tested above work when the code is run properly within an office environment and only fail with mocking.

The only workaround that worked was:

context.workbook.load('names')

Expected behaviour

I would expect any code that runs fine when not under test to work with the mock object as well.

@millerds
Copy link
Contributor

millerds commented Jan 6, 2023

Thanks for reporting this. It looks like there are 3 different issues with the mock package here.

  • The mock package doesn't support the empty 'load()' - We're expecting an argument indicating the property to load. While it is best practice, it's not required. This should be simple to fix in the package.
  • The mock package doesn't support the 'context.load(...)' call - This is a less used calling pattern that wasn't implemented. We'll have to look into adding this.
  • The mock package doesn't support office collections - The calling pattern here is a bit different since it's not just one object and so calls like workbook.names.load("type") should work such that a later call to workbook.names.items[0].type is accepted. This "nested load" is also something that needs to be added to mock package.

@ramzitannous
Copy link

ramzitannous commented Feb 3, 2023

@HarryPi @millerds any workaround for this issue, this is happening for me also getting Error: Property values needs to be present in object model before load is called. while trying to mock this

const items = [
    {
      columnIndex: 1,
      values: [[""]],
      formulas: [[""]],
      address: "Sheet1!B1",
    },
  ];
  const mock = new OfficeBddinMock.OfficeMockObject({
    areas: { items },
  });
  mock.load("areas/items/values");
  mock.sync();
  console.log(mock.areas.items[0].values);

@millerds
Copy link
Contributor

millerds commented Feb 3, 2023

@ramzitannous that is not the same thing. It's also incomplete . . . you aren't mocking the whole office-js path. On top of that I think your resulting json structure doesn't actually include an element named "items" . . . you just assigned a variable named items as part of an object named "areas" without giving it a property name in the object.

Look at https://learn.microsoft.com/en-us/office/dev/add-ins/testing/unit-testing for some usage examples.

@ramzitannous
Copy link

ramzitannous commented Feb 6, 2023

@millerds this pattern is needed for range area testing for ex. targetRangeArea.load("areas/items/values,areas/items/formulas,areas/items/address");

@millerds
Copy link
Contributor

millerds commented Feb 6, 2023

The original comment about collections is a currently limitation that needs to be addressed. Your error, however, is not the same thing even if you are trying to accomplish the same type of testing. Even if support for collects were there, you would be hitting your error because your mock structure above the collection isn't complete and the collection structure you are creating doesn't match the officejs collection structure (which is what you are trying to mock).

@ramzitannous
Copy link

@millerds the structure I'm mocking is exactly like what sheet.getRanges() returns, can you help and show what is wrong in the above code ?

@millerds
Copy link
Contributor

OK . . . looks like I was mistaken. I thought that the way you were building the structure would leave an anonymous reference to the item collection, but apparently the variable name is keep and used at the property name. It's still just a fragment of the full api structure that would be used in the actual code. The error you getting is different because you're trying to load a property of one of the items of the collection which doesn't exist in the mock object created from the data object.

In any case . . . there isn't a workaround. The Mock object doesn't handle collections/arrays and so when parsing out the data input it stops going down at the array and moves onto the next property. This is a limitation of the current mock object. We have a work item on our back log to improve this.

@ramzitannous
Copy link

@millerds is there a timeline for implementing this ?

@millerds
Copy link
Contributor

We don't comment on timelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants