Skip to content

Better errors - #97

Merged
julianrubisch merged 3 commits into
stimulusreflex:masterfrom
rickychilcott:better-errors
May 29, 2021
Merged

Better errors#97
julianrubisch merged 3 commits into
stimulusreflex:masterfrom
rickychilcott:better-errors

Conversation

@rickychilcott

Copy link
Copy Markdown
Contributor

Type of PR (feature, enhancement, bug fix, etc.)

Description

In development and test, we render a short version of the error message (and a hidden backtrace) to be able to catch typos, bugs in our partials, etc.

Fixes #63

Why should this be added

While developing you're not often watching the logs to see what's happening. This will output a fairly obvious error message and the backtrace is hidden (via display: none;

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

Comment thread lib/futurism/resolver/resources.rb Outdated

@julianrubisch julianrubisch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry it took so long. There's only one typo and a documentation request 😄

Comment thread lib/futurism/resolver/resources.rb Outdated

private

def error_render

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
def error_render
def error_renderer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm happy to correct this in the next day or two, assuming you're ok with the assert_cable_ready_operation_on extraction.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fine with it! Let's hear what the others say.

end
end

def assert_cable_ready_operation_on(stream, operation:, selector:, html:, &block)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you have a link to the respective docs and/or further explanation for me?

TBH it's hard to grok what's going on with broadcasts and clear_messages etc., and it's hard to see why this preferable to the original, simpler assert_broadcast_on

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Totally get the confusion -- I honestly don't know what it's mostly doing. This is a copy-paste job from https://github.com/rails/rails/blob/5aaaa1630ae9a71b3c3ecc4dc46074d678c08d67/actioncable/lib/action_cable/test_helper.rb#L97

With then a slight modification to build the cable-ready payload. If this style of thing works, I think extracting this to its own file (happy to do so now), and ideally extract to a gem or really just including in https://github.com/hopsoft/cable_ready might be warranted. The API would need to be cleaned up to support multiple message broadcasts and such, but could be helpful to the community.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah okay, now I get it. Yep, that makes sense. I do think that it would make sense to have this in CR, e.g. to help out here: https://github.com/hopsoft/stimulus_reflex/blob/master/test/broadcasters/page_broadcaster_test.rb#L41

Ping @leastbad @marcoroth @hopsoft

@leastbad leastbad May 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am no testing expert, but I can say that very recently I found myself copy-pasting the assert_broadcast_on method into the test class (temporarily!) so that I could redefine the method with a puts in there to spit out what the assert was expecting so I could tweak the data structure hard-coded into the test and make it pass. eg the functionality was working but the test was off by "this much"

I would love to see CR redefine assert_broadcast_on to also print out (puts) the value it's expecting.

@julianrubisch
julianrubisch merged commit f1d3188 into stimulusreflex:master May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better Error Handling

3 participants