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

initializeTestEnvironment @firebase/rules-unit-testing should not require node process.env #8795

Open
rgant opened this issue Feb 15, 2025 · 10 comments

Comments

@rgant
Copy link

rgant commented Feb 15, 2025

Operating System

macOS

Environment (if applicable)

Any Modern Browser, Chrome

Firebase SDK Version

@firebase/[email protected]

Firebase SDK Product(s)

Firestore, Storage

Project Tooling

Angular App

$ ng version

     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 19.1.7
Node: 20.18.1
Package Manager: npm 11.1.0
OS: darwin arm64

Angular: 19.1.6
... animations, common, compiler, compiler-cli, core, forms
... localize, platform-browser, platform-browser-dynamic, router
... service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1901.7
@angular-devkit/build-angular   19.1.7
@angular-devkit/core            19.1.7
@angular-devkit/schematics      19.1.7
@angular/cli                    19.1.7
@angular/fire                   19.0.0
@schematics/angular             19.1.7
rxjs                            7.8.1
typescript                      5.7.3
zone.js                         0.15.0

Detailed Problem Description

emulatorFromEnvVar assumes that it is running in a Node.js environment with access to process.env, however this test library is useful for browser based applications.

It would be nice if this method could check for process to be defined before attempting to use it. It would also be nice if initializeTestEnvironment config (TestEnvironmentConfig) would allow you to skip configurations that you don't need. For example, I am running the emulator with --only auth,firestore,storage. So if hub could figure out that I have firestore and storage but not database running and just configure those that would be less noise in my code.

Steps and code to reproduce issue

    const testEnv = await initializeTestEnvironment({
      // @firebase/rules-unit-testing assumes it is running in a node.js environment, but Angular
      // is running in a browser with webpack and so `process is not defined` will be thrown here
      // unless _every_ configuration is set so the code doesn't try to load one using environment
      // variables.
      // Also note that in order to add debugging statements to node_modules/@firebase/rules-unit-testing/dist/esm/index.esm.js
      // that will actually be picked up by webpack you need to run `ng cache clean` after every
      // change.
      database: {
        host: '125.0.0.1',
        port: 9000,
      },
      firestore: {
        host: '127.0.0.1',
        port: 8080,
      },
      hub: {
        host: '127.0.0.1',
        port: 4400,
      },
      projectId: 'brainfry-app',
      storage: {
        host: '127.0.0.1',
        port: 9199,
      },
    });

See the full app: https://github.com/rgant/brainfry/blob/47c25d00814520c4f90e7565bd9d601d2cb5c9dd/src/testing/firestore-data.ts#L28

@rgant rgant added new A new issue that hasn't be categoirzed as question, bug or feature request question labels Feb 15, 2025
@google-oss-bot
Copy link
Contributor

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

@rgant
Copy link
Author

rgant commented Feb 15, 2025

If there are no serious objections to my suggested changes, I would be happy to open a PR.

@rgant
Copy link
Author

rgant commented Feb 15, 2025

Also, doing this results in this warning when running my tests:

WARN: 'Warning: config for the database emulator is specified, but the Emulator hub reports it as not running. This may lead to errors such as connection refused.'

Which is just noise that I would prefer not to have in my logs.

@jbalidiong jbalidiong added needs-attention testing-sdk testing with emulator and removed needs-triage new A new issue that hasn't be categoirzed as question, bug or feature request labels Feb 17, 2025
@dlarocque
Copy link
Contributor

Hi @rgant, can you elaborate on why your use case requires you to run these tests in the browser?

@rgant
Copy link
Author

rgant commented Feb 19, 2025

I need to setup and check test data in Firestore without worrying about the firestore.rules in an Angular application.

For example, Quiz documents must be either owned by the currently logged in User or shared: true. And also, with rules, that query throws an permission error instead of an empty list.

So in my tests I want to have both Quizzes owned by the User logged in during testing, and Quizzes that are shared but owned by another user. Which I have done by exporting a specific setup of Quizzes in Firestore emulator.

Also, to test that empty list case, I need to un-share some Quizzes, which is easier to do with a NoRulesContext. Otherwise this test needs to login as multiple users to change the quizzes, and change them back.

@dlarocque
Copy link
Contributor

dlarocque commented Feb 19, 2025

@rgant Thanks for the detailed explanation. I believe all of this behaviour could be tested in a Node environment (please correct/explain if i'm wrong). Have you ran into issues running these tests in a Node environment?

You have mentioned multiple issues you're experiencing with rules-unit-testing. Could we keep this issue focused on the discussion of rules-unit-testing requiring process.env, and open new issues for other discussions (NoRulesContext, initializeTestEnvironment configuration, clearDatabase resetting to the default imports)? I don't want these other issues to get lost if this issue is resolved/closed.

@rgant
Copy link
Author

rgant commented Feb 19, 2025

Angular tests are done in a browser environment. That is the default for Angular testing setup at this time. IIRC, Angular plans to continue to focus on real browser testing. Using some other testing setup would require me to leave the official Angular setup, which is not possible for my project.

My review of the rules-unit-testing code shows no other node.js requirements other than in discovery.ts.

@rgant
Copy link
Author

rgant commented Feb 19, 2025

BTW, I'm not asking that the tool to not use environment variables. I'm just asking for it to not require them. If you are open to that possibility I am happy to open a PR. If you have other requirements then I would appreciate understanding them.

It could be as straightforward as adding:

function hasEnv(): boolean {
  return typeof process !== 'undefined' && 
         typeof process.env !== 'undefined';
}

@hsubox76
Copy link
Contributor

The main purpose of the rules-unit-testing library is to let you run a number of scenarios where the rules should either prevent or an allow the operation to go through, to ensure your rules are written correctly and are blocking/allowing what they should.

It seems like that's not what you're interested in testing. Please let me know if I misunderstood, but you seem to be interested in testing the app functionality itself and what you really want is to be able to use the emulators with the rules turned off. Unfortunately that's not really what the rules-unit-testing library is designed for.

If you were interested in testing your rules, we'd recommend two sets of tests - a set of app tests, as you currently have, in the browser, that test app functionality, and another set of tests, in Node, that test whether various scenarios successfully make it through your rules or not. These would be testing for two different things.

I can see how it would be convenient to have more tools that allow you to customize emulators for special cases in application testing and we can add any suggestions you have as a feature request. This isn't a code decision the SDK team can make by ourselves as it would change the nature of the product and broaden the environments this SDK supports, which needs product buy-in, but if we get enough demand (comments, upvotes on the FR issue), I'm sure that would factor into the decision.

In the meantime, some workarounds might be:

  • Write a shell script to rename firestore.rules before your app tests run and then restore it after the tests are done. This is a little hacky, but turning off all the rules to run your tests is somewhat of a hack.
  • If you feel like the change really needs to happen in the Firebase SDK, you can use patch-package to make your changes if we're not able to get around to it - seems like you already know what the changes should be and they're pretty small, so this should be a possibility

I'll mark this as a feature request in the meantime, and will keep track of how much demand we get on it.

@rgant
Copy link
Author

rgant commented Feb 19, 2025

  • Write a shell script to rename firestore.rules before your app tests run and then restore it after the tests are done. This is a little hacky, but turning off all the rules to run your tests is somewhat of a hack.

That won't work because the functionality of the rules influences the functionality of the application. With rules, the list query throws a FirebaseError. Without rules it returns an empty array.

  • If you feel like the change really needs to happen in the Firebase SDK, you can use patch-package to make your changes if we're not able to get around to it - seems like you already know what the changes should be and they're pretty small, so this should be a possibility

I already have a work around, it is just ugly. If you all do not wish to support this use case then I guess I just document and move on. But since it is a small change I hoped you would be willing to allow it.

The main purpose of the rules-unit-testing library is to let you run a number of scenarios where the rules should either prevent or an allow the operation to go through, to ensure your rules are written correctly and are blocking/allowing what they should.

Understood, but it also exposes the NoRulesContext which doesn't appear to match the primary purpose you mention. That feature is explicitly for test setup, which is how I am using it. Eventually I will probably write specific tests of the firestore.rules file. But I will also need tests of the web application. I honestly feel like I am the first person to ever use the Firebase emulator for a web application testing. There are so many quirks and missing features that are actually needed for robust testing. This is just one I could solve.

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

5 participants