Assigning @Nik-09 for the first pass review of this PR. Thanks!

Hi @ujjwalpathaak, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

Hi @ujjwalpathaak, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@seanlip @HardikGoyal2003 I was trying to mimic the behaviour of closing the tab by window.dispatchEvent(new Event('beforeunload')); and trying to spy on onClosingStoryEditorBrowserTab function to check for the desired behaviour.

In the file core/templates/pages/story-editor-page/story-editor-page.component.spec.ts like

  it(
    'should decrement number of opened story editor tabs when ' +
      'a tab is closed',
    fakeAsync(() => {
      spyOn(undoRedoService, 'getChangeCount').and.returnValue(1);
      spyOn(urlService, 'getStoryIdFromUrl').and.returnValue('story_1');
      spyOn(pageTitleService, 'setDocumentTitle');
      component.ngOnInit();

      const onClosingStoryEditorBrowserTabSpy = spyOn(
        component,
        'onClosingStoryEditorBrowserTab'
      ).and.callThrough();

      // Opening of the first tab.
      storyEditorStateService.onStoryInitialized.emit();
      // Opening of the second tab.
      storyEditorStateService.onStoryInitialized.emit();

      let storyEditorBrowserTabsInfo: EntityEditorBrowserTabsInfo =
        localStorageService.getEntityEditorBrowserTabsInfo(
          EntityEditorBrowserTabsInfoDomainConstants.OPENED_STORY_EDITOR_BROWSER_TABS,
          story.getId()
        );

      // Making some unsaved changes on the editor page.
      storyEditorBrowserTabsInfo.setSomeTabHasUnsavedChanges(true);
      localStorageService.updateEntityEditorBrowserTabsInfo(
        storyEditorBrowserTabsInfo,
        EntityEditorBrowserTabsInfoDomainConstants.OPENED_STORY_EDITOR_BROWSER_TABS
      );

      expect(
        storyEditorBrowserTabsInfo.doesSomeTabHaveUnsavedChanges()
      ).toBeTrue();
      expect(storyEditorBrowserTabsInfo.getNumberOfOpenedTabs()).toEqual(2);

      const storyId = story.getId();

      window.dispatchEvent(new Event('beforeunload'));

      tick(200);

      expect(onClosingStoryEditorBrowserTabSpy).toHaveBeenCalled();

      storyEditorBrowserTabsInfo =
        localStorageService.getEntityEditorBrowserTabsInfo(
          EntityEditorBrowserTabsInfoDomainConstants.OPENED_STORY_EDITOR_BROWSER_TABS,
          storyId
        );

      expect(storyEditorBrowserTabsInfo.getNumberOfOpenedTabs()).toEqual(1);

      // Since the tab containing unsaved changes is closed, the value of
      // unsaved changes status will become false.
      expect(
        storyEditorBrowserTabsInfo.doesSomeTabHaveUnsavedChanges()
      ).toBeFalse();
    })
  );

tried it both with fakeAsync and without fakeAsync.
Without fakeAsync - it gave false value for expect(onClosingStoryEditorBrowserTabSpy).toHaveBeenCalled(); and expect(storyEditorBrowserTabsInfo.getNumberOfOpenedTabs()).toEqual(1);

With fakeAsync -

	Error: Uncaught (in promise): TypeError: Cannot read properties of undefined (reading 'id')
	TypeError: Cannot read properties of undefined (reading 'id')
	    at Story.createFromBackendDict (core/templates/combined-tests.spec.js:66865:27851)
	    at StoryEditorStateService._updateStory (core/templates/combined-tests.spec.js:218814:52575)

i had mirrored the implementation of core/templates/pages/skill-editor-page/skill-editor-page.component.ts this is for the function onClosingSkillEditorBrowserTab... & specs for this file is not explicitly checking for the tab window close behaviour but rather the working of the function itself.

If it's been working fine is there a need to add the behaviour test for onClosingStoryEditorBrowserTab or we can skip it? as manual testing shows it's getting called on closing then tab. (shown in the demo video in PR's desc)

If it's needed to be added on both onClosingStoryEditorBrowserTabSpy & onClosingSkillEditorBrowserTab can you guide me how can i mimic the closing tab behaviour?

Hi @ujjwalpathaak, I'm sorry but I don't understand what you are asking. I can answer the parts I can understand:

• Yes, you need tests that cover your changes.
• I agree that you want to simulate that a beforeunload call does result in the effects that you want to see. Trigger that explicitly in your tests.
• You shouldn't be mirroring (duplicating?) the implementation of a non-test file in a test.
• There is some uncertainty about whether to use fakeAsync or not. I'm not sure what the answer is, you would need to debug that. Try and read the docs for fakeAsync to figure out whether it is relevant to your use case. If you run into errors then you would need to debug them, perhaps through logging and trying to figure out why there is a discrepancy. Another approach is to try and see what others have done (through searching on the Internet) to mimic a tab closure -- I'm not sure offhand how to do this and it will need research.

If you still run into issues I would encourage you to create a debugging doc (see our wiki) and we can guide you if you have open questions.

Sorry @seanlip, if my previous message was not that clear! I'll try to explain the issue with best efforts.

1. onClosingStoryEditorBrowserTab - the function that needs to be tested on beforeunload event
2. oppia/core/templates/pages/story-editor-page/story-editor-page.component.spec.ts path for the function's test file.

The issue is that it's a unit test so it's not able to mimic the window reload behavious. this sums it up correctly <- read this.

fakeAsync was not correct (from my prev msg) ignore that.

I have tried many things ~ trying to dispatch the beforeunload event and test the working of onClosingStoryEditorBrowserTab but nothing seems to work.

As per my research online - in the unit test we should test the working of the onClosingStoryEditorBrowserTab which already has a test which does that - Line: 576 - story-editor-page.component.spec.ts.

Now we just need to test westher the beforeunload event is calling the onClosingStoryEditorBrowserTab function or not. this is my actual fix - to call the function on beforeunload

Now in my last message I was referring to the onClosingSkillEditorBrowserTab function's implementation it gets triggered and works exactly like the onClosingStoryEditorBrowserTab function does on beforeunload.

This only has a test which checks for the implementation of the function (Line: 528 - skill-editor-page.component.spec.ts).
No tests are there to check if onClosingSkillEditorBrowserTab is getting called on the beforeunload event ot not.

What do I think? - If we need to add the test which checks if these function are getting called on the beforeunload event or not, We will need to write E2E test as to mimic human behaviour and cause tab close.

There is where the 2nd problems comes - I cant write or debug the E2E tests on my machine.

As you know Oppia's frontend tests face problems when on macos -

1. [BUG]: Slow Execution of Frontend Tests on Mac M1 #19894
2. e2e tests not running on Macbook M2 ( 8gb ram ) #21190

The units tests are still manageable! but E2E tests don't work at all.

I once had a word with @Akhilesh-max too.. he also suggested that E2E tests dont run on MacOS.

What I can do is maybe raise a PR to write the E2E tests for this fix, so that someone with a linux/windows machine could write them.

Do you have any suggestion as to how can we fix the tests for Macos - It can be a really huge blocker for someone who wants to contribute and is on mac.

Thanks @ujjwalpathaak. I would probably try to dispatch the event. You say an approach like this does not work? Do you have a debugging doc that digs into what happens then and why it doesn't work?

Re e2e tests ... that is a separate issue, let's not combine it with this one. I think the only thing we can do is to figure out what's blocking the e2e tests on Mac and try to fix that. Do you want to give it a try? Though that would be a separate task IMO.

Let me know if you are able to dispatch the event and write a test for this behaviour!

What i wanted to say was that if the unit tests are not able to test the desired behaviour of triggering a function on tab close correctly we can maybe open a new issue to write e2e tests for this behaviour instead of the unit tests.. so that someone else can pick it up.

I have a pending PR to open will try to look for a solution to the e2e tests on mac problem after it.

@ujjwalpathaak My previous comment still applies.

Hi @ujjwalpathaak, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!