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

fix(stdlib): fix uninitialized type casing #598

Merged
merged 6 commits into from
Jan 21, 2021
Merged

Conversation

darktasevski
Copy link
Contributor

@darktasevski darktasevski commented Dec 23, 2020

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

Copy link
Owner

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

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

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!

src/brsTypes/BrsType.ts Outdated Show resolved Hide resolved
src/brsTypes/BrsType.ts Outdated Show resolved Hide resolved
@sjbarag sjbarag added bug Any difference between this BrightScript implementation and RBI, or otherwise unexpected behavior stdlib Affects the standard library included with this BrightScript implementation labels Jan 4, 2021
@sjbarag sjbarag changed the title fix(rsg): fix uninitialized type casing #482 fix(stdlib): fix uninitialized type casing #482 Jan 4, 2021
darktasevski and others added 2 commits January 5, 2021 09:45
@darktasevski
Copy link
Contributor Author

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) 👍

* upstream/main:
  doc: Add table of scenegraph node statuses (sjbarag#607)
  v0.41.0 (sjbarag#605)
  feat(execution): create function to execute with scope (sjbarag#603)
  fix(stdlib): callFunc should always return a value (sjbarag#604)
  feat(rsg): Add isSubtype() and parentSubtype() functions to RoSGNode (sjbarag#584)
  doc: Link to NotImplemented.md in README
  doc: Remove project maturity concession
  doc: List unsupported BrightScript components, functions, and statements
@darktasevski
Copy link
Contributor Author

@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
Copy link
Owner

sjbarag commented Jan 21, 2021

@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 with TZ=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 (#612). Sorry about that!

Copy link
Owner

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @Puritanic! I really appreciate the effort you've put in 😄

@sjbarag sjbarag changed the title fix(stdlib): fix uninitialized type casing #482 fix(stdlib): fix uninitialized type casing Jan 21, 2021
@sjbarag sjbarag merged commit f1d0ace into sjbarag:main Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Any difference between this BrightScript implementation and RBI, or otherwise unexpected behavior stdlib Affects the standard library included with this BrightScript implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling type() on a roSGNode instance returns a different type name
2 participants