fix: use getConfig not process.env #232
Conversation
cintnguyen
force-pushed
the
runtime-config
branch
4 times, most recently
from
caaa9d7 to
c4350cc
Compare
Co-authored-by: Mena Hassan <[email protected]>
cintnguyen
force-pushed
the
runtime-config
branch
from
c4350cc to
4b2d65c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #232 +/- ##
==========================================
- Coverage 96.45% 96.36% -0.10%
==========================================
Files 194 194
Lines 1835 1841 +6
Branches 322 324 +2
==========================================
+ Hits 1770 1774 +4
- Misses 60 62 +2
Partials 5 5 ☔ View full report in Codecov by Sentry. |
hurtstotouchfire
added
the
needs maintainer attention
There was a problem hiding this comment.
looks good, but I have a few questions and a request for tests @cintnguyen
| @@ -41,8 +42,21 @@ export const App = () => { | |||
| const { supportEmail } = reduxHooks.usePlatformSettingsData(); | |||
| const loadData = reduxHooks.useLoadData(); | |||
|
|
|||
| const optimizelyScript = () => { | |||
| if (getConfig().OPTIMIZELY_URL) { | |||
| return <script src={getConfig().OPTIMIZELY_URL} />; | |||
There was a problem hiding this comment.
missing test; could you add one?
There was a problem hiding this comment.
Tried tackling this in 370aa3f, ran into issues with runBasicTests() not respecting dynamic getConfig mocks. It seems to be a test scope issue. I am not quite sure what refactoring of App.test.jsx would be needed to get this working yet.
I will be out of office until end of next week, in the meantime @brian-smith-tcril and @httpsmenahassan have offered to take a look at this.
| const optimizelyScript = () => { | ||
| if (getConfig().OPTIMIZELY_URL) { | ||
| return <script src={getConfig().OPTIMIZELY_URL} />; | ||
| } if (getConfig().OPTIMIZELY_PROJECT_ID) { |
There was a problem hiding this comment.
missing test; could you add one?
deborahgu
added
the
waiting on author
|
@deborahgu I added tests that cover the lines you mentioned, but it seems the overall coverage is still down from before. The only thing that stood out to me when looking at the diff between
|
There was a problem hiding this comment.
I'll gladly update whatever test was covering these lines before, I just don't know where I should be looking.
yeah, @brian-smith-tcril, I'm a little stumped by this as well. I'd think it's just the getConfig on line 77 it wants tested, but then it's complaining about 2 lines, not one.
I'm comfortable forcing this through with the codecov complaint, tbh. I won't merge until morning because I hate doing something weird when it's the end of the day, but I'm going to approve this.
deborahgu
removed
waiting on author
That part makes sense to me, it's testing the condition but it's never testing it with a frontend-app-learner-dashboard/.env.test Lines 42 to 44 in 5ca1e9d I tried adding hotjar stuff to the mocked return values for React.useEffect(() => {
// [...]
}, [authenticatedUser, loadData]);Since I was hoping I could find a way to see what test covered what lines, but I didn't see anything in the codecov web UI or the html report generated by running jest locally. My searches ("jest coverage see what test is covering a line" and "codecov see what test is covering a line") also didn't lead anywhere helpful. Hopefully I can figure that out at some point. It seems like a very useful thing when updating tests in scenarios like this.
Thank you so much! |
Changed process.env to getConfig() in order to support runtime configurations. Moved all process.env use in the index.html header to helmet.
Co-authored-by: @httpsmenahassan