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

feat: add functions_env_vars #1551

Merged
merged 8 commits into from Sep 9, 2019
Merged

feat: add functions_env_vars #1551

merged 8 commits into from Sep 9, 2019

Conversation

grant
Copy link
Contributor

@grant grant commented Aug 16, 2019

Adds devrel/samples/1063.

@grant grant requested review from kurtisvg and a team August 16, 2019 01:39
@grant grant self-assigned this Aug 16, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 16, 2019
functions/snippets/src/main/java/Env.java Outdated Show resolved Hide resolved
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class Env {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public class Env {
public class EnviromentVariableExample {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggested name is too verbose and not in line with other class names. It is not common to add Example to all the other examples. Looking at the other 3 languages, I've changed the name to EnvVars.

@Test
public void envTest() throws IOException {
new Env().env(request, response);
assertThat(responseOut.toString(), containsString(""));
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking for any empty string doesn't actually verify that this function works - any non-null string will contain and empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to set an env var in a test and tried doing so.
Do you know how to set an env var?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you stub it with Mockito?

when(System.getenv("FOO")).thenReturn("someTestValue");

Copy link
Contributor Author

@grant grant Aug 23, 2019

Choose a reason for hiding this comment

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

It looks like I can't stub with Mockito.

Cannot mock/spy class java.lang.System Mockito cannot mock/spy because : - final class

Not sure what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, it looks like we'll have to add an additional dependency to do this:

<dependency>
  <groupId>com.github.stefanbirkner</groupId>
  <artifactId>system-rules</artifactId>
  <version>1.19.0</version>
  <scope>test</scope>
</dependency>
import org.junit.contrib.java.lang.system.EnvironmentVariables;

public void EnvironmentVariablesTest {
  @Rule
  public final EnvironmentVariables environmentVariables
    = new EnvironmentVariables();

  @Test
  public void setEnvironmentVariable() {
    environmentVariables.set("name", "value");
    assertEquals("value", System.getenv("name"));
  }
}

Source: https://stackoverflow.com/a/35393623/9414803

@grant
Copy link
Contributor Author

grant commented Aug 23, 2019

Not exactly sure what to do in terms of mocking System.getenv.

@grant
Copy link
Contributor Author

grant commented Sep 8, 2019

Ok, it looks like we'll have to add an additional dependency to do this:

<dependency>
  <groupId>com.github.stefanbirkner</groupId>
  <artifactId>system-rules</artifactId>
  <version>1.19.0</version>
  <scope>test</scope>
</dependency>
import org.junit.contrib.java.lang.system.EnvironmentVariables;

public void EnvironmentVariablesTest {
  @Rule
  public final EnvironmentVariables environmentVariables
    = new EnvironmentVariables();

  @Test
  public void setEnvironmentVariable() {
    environmentVariables.set("name", "value");
    assertEquals("value", System.getenv("name"));
  }
}

Source: https://stackoverflow.com/a/35393623/9414803

Thanks for the tip and extensive suggestion to fix this. Tests are now logical and pass. (Getting some merge conflicts, but those should be trivial.)
PTAL.

Copy link
Contributor

@kurtisvg kurtisvg left a comment

Choose a reason for hiding this comment

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

LGTM, pending tests passing

@grant
Copy link
Contributor Author

grant commented Sep 9, 2019

@kurtisvg Please advise as to how to fix the Java 11 test. I don't have permission to see the errors and debug.

@kurtisvg
Copy link
Contributor

kurtisvg commented Sep 9, 2019

@grant - You should be able to access the logs by clicking Details next to the test name, and then clicking on the failed test log. This repo uses resultstore, so it should be open to the public. If it's not please let me know so we can debug.

Here is what is says atm:


  • testing functions/snippets

[ERROR] src/test/java/SnippetsTests.java:[52] (indentation) Indentation: '=' has incorrect indentation level 4, expected level should be 6.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check (default) on project functions-snippets: You have 1 Checkstyle violation. -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
Testing failed: Maven returned a non-zero exit code.

@grant
Copy link
Contributor Author

grant commented Sep 9, 2019

@grant - You should be able to access the logs by clicking Details next to the test name, and then clicking on the failed test log. This repo uses resultstore, so it should be open to the public. If it's not please let me know so we can debug.

Oh, there's a bug where the Invocation Log tab shows an inaccurate state.

Screen Shot 2019-09-09 at 16 41 44

You just need to wait a few seconds until the 15k lines of logs download and then you can see the error. A bit frustrating.

I sent feedback using the feedback tool.

@grant grant merged commit 5e4fbfe into master Sep 9, 2019
@grant grant deleted the grant_functions_env_vars branch September 9, 2019 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants