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

Unit tests that read prefs can randomly panic. #20710

Closed
emilio opened this issue Apr 29, 2018 · 0 comments
Closed

Unit tests that read prefs can randomly panic. #20710

emilio opened this issue Apr 29, 2018 · 0 comments

Comments

@emilio
Copy link
Member

@emilio emilio commented Apr 29, 2018

See for example the failure I got in #20708:

---- viewport::cascading_within_viewport_rule stdout ----
	thread 'viewport::cascading_within_viewport_rule' panicked at 'Resource reader not set.', libcore/option.rs:914:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:206
   3: std::panicking::default_hook
             at libstd/panicking.rs:216
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:400
   5: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:347
   6: rust_begin_unwind
             at libstd/panicking.rs:323
   7: core::panicking::panic_fmt
             at libcore/panicking.rs:71
   8: core::option::expect_failed
             at libcore/option.rs:914
   9: <core::option::Option<T>>::expect
             at /checkout/src/libcore/option.rs:302
  10: embedder_traits::resources::read_bytes
             at components/embedder_traits/resources.rs:20
  11: embedder_traits::resources::read_string
             at components/embedder_traits/resources.rs:24
  12: core::ops::function::FnOnce::call_once
             at components/config/prefs.rs:21
             at /checkout/src/libcore/ops/function.rs:223
  13: <lazy_static::lazy::Lazy<T>>::get::{{closure}}
             at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/lazy_static-1.0.0/src/lazy.rs:24
  14: std::sync::once::Once::call_once::{{closure}}
             at /checkout/src/libstd/sync/once.rs:227
  15: std::sync::once::Once::call_inner
             at libstd/sync/once.rs:340
  16: std::sync::once::Once::call_once
             at /checkout/src/libstd/sync/once.rs:227
  17: <servo_config::prefs::PREFS as core::ops::deref::Deref>::deref
             at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/lazy_static-1.0.0/src/lazy.rs:23
             at /home/servo/buildbot/slave/linux-dev/build/<__lazy_static_internal macros>:13
             at /home/servo/buildbot/slave/linux-dev/build/<__lazy_static_internal macros>:14
  18: style_tests::viewport::test_viewport_rule
             at tests/unit/style/viewport.rs:48
  19: style_tests::viewport::cascading_within_viewport_rule
             at tests/unit/style/viewport.rs:198
  20: style_tests::__test::TESTS::{{closure}}
             at tests/unit/style/viewport.rs:194
  21: core::ops::function::FnOnce::call_once
             at /checkout/src/libcore/ops/function.rs:223
  22: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1452
             at /checkout/src/libcore/ops/function.rs:223
             at /checkout/src/liballoc/boxed.rs:635
  23: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:101

This test reads a pref to see if the viewport rule is enabled, but #20533 made it so that only calls to PREFS.set were guarder by register_resources_for_tests.

register_resources_for_tests is not great: any patch that adds something that reads a pref on a code path which is unit-tested may make tests panic randomly.

@paulrouget, this is a regression from #20533, mind looking into it? I think we could have a "test" feature for the resources dir that did what register_resources_for_tests lazily... Or something.

cc @mbrubeck

@emilio emilio changed the title Unit tests that read prefs can unexpectedly panic. Unit tests that read prefs can randomly panic. Apr 29, 2018
bors-servo added a commit that referenced this issue May 11, 2018
Automatically provide a resource reader for tests

Fix #20710

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20710 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20718)
<!-- Reviewable:end -->
@jdm jdm mentioned this issue May 18, 2018
6 of 7 tasks complete
bors-servo added a commit that referenced this issue May 18, 2018
Automatically provide a resource reader for tests

Fix #20710

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20710 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20718)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 18, 2018
Automatically provide a resource reader for tests

Fix #20710

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20710 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20718)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

1 participant
You can’t perform that action at this time.