sjbarag / brs

An interpreter for the BrightScript language that runs on non-Roku platforms.
MIT License
113 stars 43 forks source link

fix(stdlib): fix uninitialized type casing #598

Closed darktasevski closed 3 years ago

darktasevski commented 3 years ago

This MR fixes the casing of the <uninitialized> brightscript type, and is related to this issue #482

Apparently, when we try to compare the type of some value to "<UNINITIALIZED>" in Brightscript running on Roku devices, it's causing the channel executing that code to crash, as it seems that this should be in all lowercase, which is working as expected.

The problem I'm having with this is that we have some tests which are running in brs and in those tests we're testing a util function which is checking if the value is valid and defined:

function isValidVal(val) as boolean
    if ((type(val) = "<uninitialized>") or (val = invalid) or (type(val) = "roInvalid"))
        return false
    end if
    return true
end function

If I test this function in brs like this:

result = isValidVal(not_defined_anywhere)

This returns true, which is not correct, but If I change the above function to check for "<UNINITIALIZED>" instead it returns the correct result which is false. The problem is that "<UNINITIALIZED>" is not the correct type and I can't refactor the function above to check for it as it's gonna cause a crash for my app.

That being said, I'm not sure if there is a particular reason why is this named "<UNINITIALIZED>", first time contributing to this project.

fixes #482

darktasevski commented 3 years ago

Hey @Puritanic — thanks for the PR! You're right, something's definitely not correct in brs. Looks like there's a little more work to align with the print variableThatDoesNotExist output in the reference implementation, but this is a fantastic start. Thanks for all your effort so far!

I'll try to fix this today (if I have time) 👍

darktasevski commented 3 years ago

@sjbarag I've fixed tests and refactored code based on suggestions you've provided, I hope that everything is okay now.

FYI, I'm getting some weird issues when running tests locally:

Summary of all failing tests
 FAIL  test/brsTypes/components/RoDateTime.test.js
  ● RoDateTime › methods › getLastDayOfMonth › returns the date/time value's last day of the month

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Int32 {
        "kind": 4,
    -   "value": 31,
    +   "value": 30,
      }

      210 |                 let result = getLastDayOfMonth.call(interpreter);
      211 |                 expect(getLastDayOfMonth).toBeTruthy();
    > 212 |                 expect(result).toEqual(new Int32(31));
          |                                ^
      213 |             });
      214 |         });
      215 |

      at Object.toEqual (test/brsTypes/components/RoDateTime.test.js:212:32)
      at processTicksAndRejections (node:internal/process/task_queues:93:5)

 FAIL  test/e2e/BrsComponents.test.js (5.228 s)
  ● end to end brightscript functions › components/roDateTime.brs

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

    @@ -20,11 +20,11 @@
        "Minutes: ",
        "14",
        "Seconds: ",
        "15",
        "Last Day of Month: ",
    -   "30",
    +   "29",
        "Milliseconds: ",
        "160",
        "ISO String UTC: ",
        "2010-11-12T13:14:15Z",
      ]

      81 |         await execute([resourceFile("components", "roDateTime.brs")], outputStreams);
      82 |
    > 83 |         expect(allArgs(outputStreams.stdout.write).filter((arg) => arg !== "\n")).toEqual([
         |                                                                                   ^
      84 |             "Full Date: ",
      85 |             "Friday November 12, 2010",
      86 |             "No Week Day: ",

      at Object.toEqual (test/e2e/BrsComponents.test.js:83:83)

These two tests are failing when I run the yarn test locally, but seems to be passing in CI 🤔

sjbarag commented 3 years ago

@sjbarag I've fixed tests and refactored code based on suggestions you've provided, I hope that everything is okay now.

FYI, I'm getting some weird issues when running tests locally:

Summary of all failing tests
 FAIL  test/brsTypes/components/RoDateTime.test.js
  ● RoDateTime › methods › getLastDayOfMonth › returns the date/time value's last day of the month

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Int32 {
        "kind": 4,
    -   "value": 31,
    +   "value": 30,
      }

      210 |                 let result = getLastDayOfMonth.call(interpreter);
      211 |                 expect(getLastDayOfMonth).toBeTruthy();
    > 212 |                 expect(result).toEqual(new Int32(31));
          |                                ^
      213 |             });
      214 |         });
      215 |

      at Object.toEqual (test/brsTypes/components/RoDateTime.test.js:212:32)
      at processTicksAndRejections (node:internal/process/task_queues:93:5)

 FAIL  test/e2e/BrsComponents.test.js (5.228 s)
  ● end to end brightscript functions › components/roDateTime.brs

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

    @@ -20,11 +20,11 @@
        "Minutes: ",
        "14",
        "Seconds: ",
        "15",
        "Last Day of Month: ",
    -   "30",
    +   "29",
        "Milliseconds: ",
        "160",
        "ISO String UTC: ",
        "2010-11-12T13:14:15Z",
      ]

      81 |         await execute([resourceFile("components", "roDateTime.brs")], outputStreams);
      82 |
    > 83 |         expect(allArgs(outputStreams.stdout.write).filter((arg) => arg !== "\n")).toEqual([
         |                                                                                   ^
      84 |             "Full Date: ",
      85 |             "Friday November 12, 2010",
      86 |             "No Week Day: ",

      at Object.toEqual (test/e2e/BrsComponents.test.js:83:83)

These two tests are failing when I run the yarn test locally, but seems to be passing in CI 🤔

Those are both date/time-based issues! Github reports your location as Serbia, so I wouldn't be surprised if our tests are a little flaky — most contributors are in UTC-7 / UTC-4, and the GitHub Actions runners probably use UTC as their timezone.

In fact, I'm able to reproduce that failure by running tests withTZ=Asia/Krasnoyarsk yarn test! You may want to run those tests locally with TZ=UTC yarn test, but I'll file an issue to figure out why those are failing with TZ=Asia/Krasnoyarsk (https://github.com/sjbarag/brs/issues/612). Sorry about that!