re-enable hh_decl:client test
commitee9e372d10b8993583449798ffd95d31febb333e
authorLucian Wischik <ljw@fb.com>
Wed, 14 Sep 2022 22:46:22 +0000 (14 15:46 -0700)
committerFacebook GitHub Bot <facebook-github-bot@users.noreply.github.com>
Wed, 14 Sep 2022 22:46:22 +0000 (14 15:46 -0700)
tree6cee69bf528a3655d74ee34f234f55d993d59a07
parent0044f2fefb35d1213823afa5c9c9c2d1cd98268b
re-enable hh_decl:client test

Summary:
I believe that the previous diff D39417809 has removed potential deadlock (hence, timeout/flakiness) from hh_decl tests. I'm therefore going to re-enable them. Fingers crossed.

-----------

jakebailey observed "IIRC the failures involved something missing from the filesystem; could you add .context() calls in the test instead of unwraps so that (if indeed it's the filesystem and not deadlocks) we can get signal on where the failure is?"

So I spent care making sure we got good callstacks. My findings:
1. If we write `fn test() -> Result<()>` and it results in error, cargo test will :? print the error, but buck test will merely say "0!=1" and show a useless callstack.
   * **SOLUTION: our tests should never return Result; they should unwrap() instead.**
2. If we convert HhError to Anyhow, or Result<T,HhError> to Result<T,Anyhow> then the callstack gets lost.
   * **SOLUTION: (1) this will be fixed when we upgrade thiserror, anyhow and rustc. (2) we bypass this issue if tests use unwrap() rather than converting to anyhow.**
3. Optimizations were disrupting callstacks.
   * SOLUTION: **#[inline(never)] fixed it.**

Here's the optimization issue I observed.
```
#[test] fn test() {f().unwrap();}

fn f() -> Result<(), HhError> {
  io().hh_context("c")?;
  Ok(())
}

fn hh_context(self, context: &'static str) -> Result<T, HhError> {
    self.map_err(|err| HhError::Unexpected(anyhow::Error::new(err).context(context)))
}
```
If the io() call fails, I want a callstack "test > f#io" which points me to the line that does the io command. But instead I was getting a callstack "test > hh_context#closure". My hypothesis is that (1) fn hh_context was being inlined into f and its callback is being executed in the same stack frame as f, (2) to generate the backtrace we look at the current machine instruction in the current frame and see that the current machine instruction is associated with hh_context:closure, and the parent stack frame is associated with fn test, and that's why we print "test > hh_context#closure".

Use of #[inline(never)] meant that the bottom frame is fn hh_context, and the next frame up is fn f at the call to .hh_context, and that's why we now get good backtraces.

---------

I actually did observe a test failure due to missing files. https://www.internalfb.com/intern/sandcastle/job/13510799651815164
```
thread 'test_persistent_client_creation_deadlock' panicked at
'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }',
hphp/hack/test/facebook/hh_decl/client_tests.rs:85:65
```

It doesn't make sense to me. We fetching env var "ROOT" and calling std::fs::canonicalize on it, and the attempt to canonicalize is what's reporting "No such file or directory".

```
rust_unittest(
    name = "client",
    env = {"ROOT": "$(location //hphp/hack/test/integration/data/simple_repo:repo)"}
)

buck_filegroup(
    name = "repo",
    srcs = glob(["**/*.php", "**/.hhconfig"])
)
```

But how can ROOT ever point to a non-existent directory?

What I'd like to do in such cases skip the test. Failing that, I'd like to print a message and succeed. But both options appear to be unavailable from rust tests. Therefore instead what I'll do is succeed silently in this hard-to-repro case.

Differential Revision: D39418730

fbshipit-source-id: cbd09f8820335f5aae98e80a32468b8f9eed8274
hphp/hack/src/utils/hh24_types/hh24_types.rs